Skip to content
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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

vivianlin2000
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Jul 5, 2020

Codecov Report

Merging #523 into master will decrease coverage by 0.00%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
regolith/builders/grantreportbuilder.py 56.52% <56.52%> (ø)
regolith/builder.py 100.00% <100.00%> (ø)
regolith/dates.py 88.19% <0.00%> (+0.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16c3c25...6546f6e. Read the comment docs.

Copy link
Contributor

@sbillinge sbillinge left a 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.

Copy link
Contributor

@sbillinge sbillinge left a 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:
Copy link
Contributor

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"]:
Copy link
Contributor

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":
Copy link
Contributor

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:
Copy link
Contributor

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?

@vivianlin2000 vivianlin2000 changed the title WIP: Regolith Proposal Report Builder WIP: Regolith Grant Report Builder Jul 19, 2020
Copy link
Contributor

@sbillinge sbillinge left a 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:
Copy link
Contributor

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']]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) nice!

Copy link
Contributor

@sbillinge sbillinge left a 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("-")
Copy link
Contributor

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....

Copy link
Collaborator Author

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.

Copy link
Contributor

@sbillinge sbillinge left a 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")
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

@sbillinge sbillinge left a 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!

@@ -1,9 +1,8 @@
NSF {{beginYear}}-{{endYear}} Annual Report by {{authorFullName}}, ({{ institution }})
NSF {{beginYear}}-{{endYear}} Annual Report by Simon J. L. Billinge, (Columbia University)
Copy link
Contributor

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):
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

@sbillinge sbillinge left a 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:
Copy link
Contributor

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)
Copy link
Contributor

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
Copy link
Contributor

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?

@@ -1,4 +1,4 @@
NSF {{beginYear}}-{{endYear}} Annual Report by Simon J. L. Billinge, (Columbia University)
NSF {{beginYear}}-{{endYear}} Annual Report
Copy link
Contributor

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?
#############################################################
Copy link
Contributor

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?

Copy link
Contributor

@sbillinge sbillinge left a 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")
Copy link
Contributor

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.

  1. require the person id (e.g., you would be vlin). This is easier to remember because we use a mostly standard naming scheme.
  2. 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)
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants