-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Regolith Grant Report Builder #523
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #523 +/- ##
==========================================
- Coverage 65.57% 65.57% -0.01%
==========================================
Files 59 60 +1
Lines 5531 5560 +29
==========================================
+ Hits 3627 3646 +19
- Misses 1904 1914 +10
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to see this started! Already one request. I realize I made a mistake.....I don't have to make reports on my proposals, but on my grants, so it should be GrantReportBuilder, etc.
The normal way to do builders is to start with the report that is being build and to work back. If you want to have a quick meeting, that would work great. The closest builder may be the cv or the appraisal builder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Vivian, this looks great.
Pls see my comments.
Also another comment but I can't leave it inline for some reason, but load the gtx collections by looping over the needed_dbs
list.
I will have more time starting next week, and maybe we can have a meeting and go over the different categories and where we are going to get the information from.....
# Major Goals | ||
|
||
# Accomplishments | ||
|
||
# Opportunities for Training and Professional Development | ||
valid_presentations = [] | ||
for p in self.gtx["people"]: | ||
if p["active"] and p["_id"] is not "sbillinge": | ||
for p in people: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for person in people
|
||
# Get All Active Members | ||
people = [] | ||
for p in self.gtx["people"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current_members = [person for person in self.gtx['people'] if person['active']]
for p in self.gtx["people"]: | ||
if p["active"] and p["_id"] is not "sbillinge": | ||
for p in people: | ||
if p["_id"] is not "sbillinge": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my name shouldn't be hard-coded in here
@@ -70,6 +77,12 @@ def latex(self): | |||
|
|||
# Plans for Next Reporting Period to Accomplish Goals | |||
|
|||
# Individuals that have worked on project | |||
individuals_data = [] | |||
for p in people: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you keep looping over people so why not put each of these actions inside a single outer loop over people?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
all_docs_from_collection(rc.client, "institutions"), key=_id_key | ||
) | ||
|
||
for dbs in rc.needed_dbs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so pretty! 👍
for p in self.gtx["people"]: | ||
if p["active"]: | ||
people.append(p) | ||
current_members = [person for person in self.gtx['people'] if person['active']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice progress. Pls see comment....
begin_year, begin_month, begin_day = int(today[0]) - 1, int(today[1]), int(today[2]) | ||
end_date = date(end_year, end_month, end_day) | ||
begin_date = date(begin_year, begin_month, begin_day) | ||
start_date = rc.start_date.split("-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is heading us back down a road to the old days (passing dates around as strings and ints) rather than the brave new world of working with dates. I very much doubt this is the direction we want to go in here....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I realized this too! Will work with date objects instead of the strings and ints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls see comments
end_year, end_month, end_day = int(end_date[0]), int(end_date[1]), int(end_date[2]) | ||
end_date = datetime.datetime(end_year, end_month, end_day) | ||
begin_date = datetime.datetime(begin_year, begin_month, begin_day) | ||
start_date = datetime.strptime(rc.start_date, "%Y-%m-%d") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually use a date parser from dateutils. pls could you look in one of the other helpers for the syntax?
prums = [prum for prum in self.gtx['projecta'] | ||
if grant_name in prum['grants'] | ||
and | ||
((start_date <= datetime.strptime(prum['end_date'], "%Y-%m-%d") <= end_date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use get_dates to get dates and is_current etc. (fully tested functions in tools.py to do this work if poss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice....coming together! Very exciting!
regolith/templates/grantreport.txt
Outdated
@@ -1,9 +1,8 @@ | |||
NSF {{beginYear}}-{{endYear}} Annual Report by {{authorFullName}}, ({{ institution }}) | |||
NSF {{beginYear}}-{{endYear}} Annual Report by Simon J. L. Billinge, (Columbia University) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shouldn't be hard-coded in here.
Specific Objectives: | ||
Significant Results: | ||
------------------------------------------------------------------------------ | ||
- Major Activities (currently worked on projecta): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, can't have me hard-coded in here.
If there is something that is "me" specific that doesn't change often it should probably be in people. However, since this is a grant-specific thing it might better be in grants? If it is mostly me-specific but will be described in the grant report of multiple grants, this should probably be in people but have a field that is grant
. This is how we discover how the world is organized!
{% endfor -%}} | ||
|
||
* How have the results been disseminated to communities of interest? | ||
1. Publications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting one. It should probably be in people too so any user of Regolith can decide for themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice progress! I left a few comments.
What is the progress. I wonder if I should merge this sometime soon and try and use it and give usage feedback?
# How have results been disseminated | ||
for education in person['education']: | ||
if 'phd' in education['degree'].lower() and 'columbia' in education['institution'].lower() and \ | ||
rp_start_date.year <= education['end_year'] <= rp_end_date.year: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you will be doing something like this a lot I guess, you may want to use a function. Can you use is_current which has already been written? I would probably try and use get_dates to get dates and then is_current.
It may not work, but using functions that are well tested and can handle edge cases well will save time later.
# Participants/Organizations | ||
participants = {} | ||
for person in grant_people: | ||
p = self.gtx["people"].get(person) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't be lazy with variable names, it makes the code hard to read. What is p?
Here I think the logic is that p contains the person document for the person of name person, so I would pick variable names like this:
for name in grant_people:
person = self.gtx["people"].get(person)
I think this logic assumes that the names in grant_people are valid id's is that right? This might be a bit brittle.
p = self.gtx["people"].get(person) | ||
months = 0 | ||
if p['active']: | ||
difference = datetime.today() - rp_start_date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it correct to do this calculation from today?
regolith/templates/grantreport.txt
Outdated
@@ -1,4 +1,4 @@ | |||
NSF {{beginYear}}-{{endYear}} Annual Report by Simon J. L. Billinge, (Columbia University) | |||
NSF {{beginYear}}-{{endYear}} Annual Report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you want a name here (just not my name hard-coded)
@@ -51,32 +51,17 @@ Key outcomes or Other achievements: | |||
3. Presentations at conferences and seminars | |||
|
|||
* What do you plan to do during the next reporting period to accomplish the goals? | |||
############################################################# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like copy-pasted text....maybe assign this string to a variable and use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good progress. Should we have a quick chat about this one of these days?
@@ -23,6 +23,7 @@ | |||
|
|||
|
|||
def subparser(subpi): | |||
subpi.add_argument("authorFullName", help="full name of the author of this grant report") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is very brittle. I doubt I will ever remember the full name of people and whether it is capitalized and spaces and whatnot. We have two choices here.
- require the person id (e.g., you would be vlin). This is easier to remember because we use a mostly standard naming scheme.
- allow anything that is in id, name or alias and then use fuzzy retrieval to get the person back.
(2) is probably better because (1) is also covered by (2)
months = difference.year * 12 + difference.month | ||
else: | ||
for name in grant_people: | ||
person = self.gtx["people"].get(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to get things from a collection we use things like fuzzy_retrieval
in tools.py or find_one
etc. in fsclient
and mongoclient
You might also find get_person
from tools.py to be useful.
No description provided.