ENH: add public and professional summary when f_prum #657
Conversation
Codecov Report
@@ Coverage Diff @@
## master #657 +/- ##
==========================================
- Coverage 68.12% 68.09% -0.04%
==========================================
Files 67 67
Lines 6705 6717 +12
==========================================
+ Hits 4568 4574 +6
- Misses 2137 2143 +6
Continue to review full report at Codecov.
|
| @@ -60,6 +64,16 @@ def db_updater(self): | |||
| print("Please rerun the helper specifying the complete ID.") | |||
| return | |||
| found_projectum.update({'status':'finished'}) | |||
| if rc.pub_sum: | |||
| found_projectum['deliverable'].update({'public_summary': rc.pub_sum}) | |||
| elif found_projectum['deliverable'].get('public_summary') is not None: | |||
There was a problem hiding this comment.
I think the logic should probably be:
if not found_prum['professional_summary']:
if rc.pro_sum:
found_prum['professional_summary'] = rc.pro_sum
else:
raise RuntimeError("ERROR: please rerun with a professional summary in field --pro_sum and public summary in --pub_sum")
and
if not found_prum['public_summary']:
if rc.pub_sum:
found_prum['professional_summary'] = rc.pub_sum
else:
raise RuntimeError("ERROR: please rerun with a professional summary in field --pro_sum and public summary in --pub_sum")
There was a problem hiding this comment.
Hi Simon, I am a bit hesitate to change the logic into this way, because once we update the schema.py and a_projectumhelper.py, when people create a new prum, the professional_summary field will automatically have some words there (because we initize it).
In this situation, the first if statmentif not found_prum['public_summary']: is useless.
There was a problem hiding this comment.
We should prevent people entering random stuff or not changing the original initialization words in the professional_summary field when people finish a prum.
| print("Error: please enter the public summary by specifying pub_sum argument.") | ||
| return | ||
| found_projectum['public_summary'] = rc.pub_sum | ||
| elif found_projectum.get('public_summary') is not None: |
There was a problem hiding this comment.
I think this logic is not quite right, please see my code from the previous convo. The outer loop should check if the summaries exist in the db or not and it should not overwrite them if they do. Then it should check if rc.pub_sum exists and use that, finally it should raise a runtime error if it neither already exists in the db nor has been given to the cli.
We should also catch the case where it finds something in the db and is given a value on the cli with a f"WARNING: public summary {found_projectum.get('public_summary')} exists in the db and will not be overwritten with {rc.pub_sum} given on the command line. If you want to force this change please rerun adding --force" or sthg like that.
|
OK, I may be wrong, but I am not sure. A test will make me sure.....
…On Fri, Dec 18, 2020 at 7:43 AM longyang ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In regolith/helpers/u_finishprumhelper.py
<#657 (comment)>:
> @@ -70,16 +70,27 @@ def db_updater(self):
print("Please rerun the helper specifying the complete ID.")
return
found_projectum.update({'status':'finished'})
- if rc.pub_sum:
- found_projectum['public_summary'] = rc.pub_sum
- elif found_projectum.get('public_summary') is not None:
+
+ if found_projectum['professional_summary']:
Umm, I think the logic should work according to our yesterday discussion.
I handled all the four conditions in my codes. If both prof_sum and
rc.pub_sum, it will just raise error and ask user to type again.
Yup sure, I will make a test function.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#657 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAOWUI5LQIOE5LJ6TKBYHDSVNE53ANCNFSM4UPIAGNA>
.
--
Simon Billinge
Professor, Columbia University
Physicist, Brookhaven National Laboratory
|
|
haha, this is why we write tests! :) (and do code review, but the review is easier if there are good tests!) |
|
btw, if you write a good function, we can abstract it to be used more widely. This kind of "syncing" is presumably quite common in our helpers. So I encourage you to use variable names that are not specific to this UC. |
|
@dragonyanglong I am not sure where you are with this, but I have had a change of thought. I am wondering, should we put the public and professional summaries into the paper entry in the citations db? I think yes. This means that the summaries/descriptions follow the paper around and we don't have to dereference a projectum for each paper. Most papers will have a prum, but we don't have an "associated_prum" field in the citation so there is currently no way to find the summaries for a paper just from the paper, which doesn't make sense to me. Actually, come to think of it we shoujld probably have a None of this changes much what is happening in this PR, except that we want to push the prof and pub summaries to the ciatation and not the prum. What do you think? |
|
Hi Simon, yup, I agree. make these summaries attached with papers sound reasonable. I will migrate the changes to citations. For the |
|
and BTW, it seems like there is no helper functions for citataions like |
|
Thanks Long. This is because I was going to change the way we handled
citations and just store the doi and any metadata, but I realize this won't
work for "in progress"publications so I am not sure what to do. Bottom
line, I didn't want anyone spending slot of time writing helpers that would
go out of date. But having a helper that inserts a prum_id and the
summaries is still good.
…On Sat, Dec 19, 2020, 8:16 AM longyang ***@***.***> wrote:
and BTW, it seems like there is no helper functions for citataions like
f_prum etc.... And for citations, usually we just create one, then
manually update the info when published (like updating the final page
numbers etc). So the workflow is bit different from prums.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#657 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAOWULYGBHZMSDA45YBERDSVSRSHANCNFSM4UPIAGNA>
.
|
@sbillinge , as we discussed. We want people mandatory to input the public and professional summary when finishing a prum.
Currently, I didn't update the
schema.pyanda_projectumhelper.py, because I don't know how to initialize these two entries. If we initialize them, it is difficult afterwards for us to check whether people really input something useful for the summaries, or just random useless str when f_prum.Please review.