Added timestamp calculation to detsim city #778

MCruces-fz · 2021-03-16T16:05:35Z

Timestamp calculation is based on rate parameter and event number.

MCruces-fz · 2021-03-16T16:06:26Z

I have left the modifications of the names of the output files for each detsim test in a different commit at the request of @mmkekic.
All other changes pertinent to the calculation of the timestamp are in the last commit

invisible_cities/detsim/sensor_utils.py

invisible_cities/detsim/sensor_utils_test.py

invisible_cities/cities/components.py

jacg · 2021-04-06T14:03:12Z

The tests are failing. It is not immediately obvious to me that the failure is spurious.

If the failure is spurious, please rerun the job on IC.
If the failure is not spurious, please fix: having broken PRs sitting around in the queue is not helpful.

jacg · 2021-04-06T14:04:37Z

... and please rebase on top of #783 as soon as it is merged, so that we get clearer test error reporting.

jacg · 2021-04-06T14:29:24Z

Is there any reason why this PR should not be reviewed and merged?

jacg · 2021-04-06T14:55:41Z

#783 has been merged. Please rebase.

MCruces-fz · 2021-04-06T15:01:40Z

I should clear the commit history a bit. Then, can we merge it?
Maybe I should do more modifications

gondiaz · 2021-04-06T15:04:45Z

This is the warning message I get when running the function in a python shell:

>>> f = create_timestamp(-2*units.hertz)
/Users/gonzalo/NEXT/IC/invisible_cities/cities/components.py:137: UserWarning: Negative rate is unphysical, using rate = 2.0 Hz instead
  warnings.warn(f"Negative rate is unphysical, using "

Shouldn't we add stacklevel=2 to the warning? The output would be clearer I think

>>> f = create_timestamp(-2*units.hertz)
__main__:1: UserWarning: Negative rate is unphysical, using rate = 2.0 Hz instead

jacg · 2021-04-06T15:19:30Z

I should clear the commit history a bit.

Yes, but there's no point in doing that until you have an approving review ... UNLESS (and it's a very important 'unless') cleaning up history will help the reviewers understand what you have done.

Don't hesitate to ask for help with the cleaning, if you need it.

Then, can we merge it?

I'm not the best person to answer that question. You should probably ask for a review (using the buttons/links on the right, near the top of the PR page) from the people you consider most familiar with the issue you are resolving.

MCruces-fz · 2021-04-06T15:24:29Z

Shouldn't we add stacklevel=2 to the warning? [...]

Good idea, I add that now

mmkekic · 2021-04-06T15:29:33Z

I'm not the best person to answer that question. You should probably ask for a review (using the buttons/links on the right, near the top of the PR page) from the people you consider most familiar with the issue you are resolving.

Well reviewer @gondiaz has already been assigned and it seems he is mostly happy with the changes.

Shouldn't we add stacklevel=2 to the warning? [...]

I think this is a good idea, that way the warning will point to the function calling create_timestamp where the nonphysical rate value is passed, instead of pointing to the create_timestamp itself

MCruces-fz · 2021-04-06T15:29:33Z

Okay, thank you @jacg

jacg · 2021-04-06T15:41:11Z

Well reviewer @gondiaz has already been assigned and it seems he is mostly happy with the changes.

@gondiaz please formally approve once you are happy, so that it's clear when we can merge.

[@carmenromo You should start thinking about training a successor for the role of merge-master]

carmenromo · 2021-04-06T15:47:15Z

Why? Are you not happy with me?
I think most of the people know how to merge, but I can teach anyone :)

gondiaz · 2021-04-06T16:02:46Z

The PR will be ready after history rewriting/squashing. The commits should be similar to:

Modify create_timestamp function
Move create_timestamp function to components.py
Add test for create_timestamp function
Modify detsim city and source with new create_timestamp function
Modify detsim tests for new create_timestamp function
Modify buffy city and source with new create_timestamp function

Note that you usually start with a past tense at the start of commit messages, change it to present.

jacg · 2021-04-06T16:10:47Z

Why? Are you not happy with me?

Very happy!

I think most of the people know how to merge, but I can teach anyone :)

I'm sure that there are many who can merge, but far fewer are aware of the standards of merge commits we expect in IC.

Also remember that the role of merge-master serves two purposes: the idea is that some junior contributor

become comfortable with managing this not-entirely-trivial git operation, and have a regular, not too difficult, but nonetheless important job to do, in order to strengthen a sense of belonging;
relieve more senior members of the somewhat mechanical task.

I think that you're waaaaay past needing point 1 for yourself.

If there are any newish team members who would like a little bit of additional responsibility, or an opportunity to get a bit more comfortable with Git, then step forward.

Job description: merge approved PRs as soon as they are ready, following the merge commit style that we have established in IC. Sometimes PRs are merged by senior people involved, but sometimes they rush on to other things. We need someone who will make sure that approved PRs don't stay unmerged for more than 24h (prefereably 24 minutes :-)

carmenromo · 2021-04-06T16:16:44Z

Nice! I see and I fully agree. Do you have someone in mind?

jacg · 2021-04-06T16:17:44Z

Note that you usually start with a past tense at the start of commit messages, change it to present.

Strictly speaking it's the imperative mood, rather than present tense:

Do this, fix that, remove the other ...

Some people find it helpful to check whether the commit title completes a sentence starting with something like "Including this commit will ..."

For example: Including this commit will ... fix the bug that cost us the Nobel Prize

jacg · 2021-04-06T16:19:03Z

Do you have someone in mind?

Nope. I have no idea who is lurking in the background, how they feel about their relationship to the project etc. etc.

... maybe, after the pandemic is over and I meet some of the new people ...

carmenromo · 2021-04-06T16:22:22Z

@halmamol has volunteered!

jjgomezcadenas · 2021-04-06T16:22:29Z

What about Miryam?

…

On 6 Apr 2021, at 18:19, Jacek Generowicz ***@***.***> wrote: Do you have someone in mind? Nope. I have no idea who is lurking in the background, how they feel about their relationship to the project etc. etc. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#778 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5SID6AZ43PSVZVSRQJ6IDTHMYANANCNFSM4ZI265SQ>.

jjgomezcadenas · 2021-04-06T16:24:27Z

Ja! The usual suspects! @halmamol gets the job!

…

On 6 Apr 2021, at 18:22, carmenromo ***@***.***> wrote: @halmamol <https://github.com/halmamol> has volunteered! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#778 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5SID4GS4LPQDQQIEG4TFLTHMYM3ANCNFSM4ZI265SQ>.

carmenromo · 2021-04-06T16:29:11Z

If @MiryamMV wants I can teach both and they can agree as I did with @bpalmeiro.

jacg · 2021-04-06T16:33:13Z

Hi @halmamol,

I'm sorry, I don't know who you are. I hope that the coronavirus will allow us to remedy this in the not too distant future. In the meantime don't hesitate to reach out to me if you want any help or assistance, though I am confident that @carmenromo will be able to get you up to speed without any trouble.

jacg · 2021-04-06T16:34:27Z

Hi @MiryamMV,

My last message applies to you too!

jacg · 2021-04-06T17:08:10Z

Sorry @MCruces-fz ... we seem to have hijacked your PR's conversation to discuss the merge-master role.

@carmenromo, @halmamol, @MiryamMV I'll refrain from merging #784 (which is still waiting for @jmbenlloch's approval, but should be ready), to leave you with plenty of material for practice. Please merge that one before any of the others, otherwise the spurious test failures will drive you nuts. (The whole PR is just a single commit, but do add a merge commit anyway.)

Next is #780. It's squashed and approved, but wait until #784 is in.

After that you can expect this one (#778) and #781 to be ready soon, so there should be plenty of material for you to work with. Merry merging!

Added a check for physical rate values and keept the timestamp calculation nested.

- Add rate variable (0.5 Hz) to detsim.conf - Add rate parameter in detsim function and MC_hits_from_files call

Update output files of test_detsim_exact and test_buffy_exact_result

MCruces-fz · 2021-04-07T15:33:06Z

I hadn't realized that my branch was outdated

gondiaz

This PR succesfully implements timestamps for MC events in both detsim and buffy cities. Good job!
You can proceed with merging @carmenromo @halmamol @MiryamMV.

carmenromo · 2021-04-07T16:09:17Z

Since the PR #780 is waiting to pass the tests and this PR is already rebased, can we merge this one first @jacg?

jacg · 2021-04-07T16:10:58Z

Yup.

mmkekic requested a review from gondiaz March 16, 2021 17:30

MCruces-fz force-pushed the detsim_timestamp branch from cecf8f6 to adc116d Compare March 16, 2021 17:38

gondiaz reviewed Mar 16, 2021

View reviewed changes

invisible_cities/detsim/sensor_utils.py Outdated Show resolved Hide resolved

gondiaz reviewed Mar 16, 2021

View reviewed changes

invisible_cities/detsim/sensor_utils.py Outdated Show resolved Hide resolved

gondiaz reviewed Mar 16, 2021

View reviewed changes

invisible_cities/detsim/sensor_utils_test.py Outdated Show resolved Hide resolved

MCruces-fz force-pushed the detsim_timestamp branch from 20b649f to 13a779a Compare March 18, 2021 16:46

gondiaz reviewed Mar 23, 2021

View reviewed changes

invisible_cities/cities/components.py Show resolved Hide resolved

MCruces-fz force-pushed the detsim_timestamp branch from 8a3c796 to 4959384 Compare April 6, 2021 15:00

MCruces-fz force-pushed the detsim_timestamp branch from 58e1993 to d10efec Compare April 7, 2021 15:28

MCruces-fz added 7 commits April 7, 2021 17:30

Modify output filename of all tests in detsim_test

7bab2bb

Modify create_timestamp function

34c437d

Added a check for physical rate values and keept the timestamp calculation nested.

Move create_timestamp function to components.py

391d845

Add test for create_timestamp function

70e6fd7

Modify detsim city and source with new create_timestamp function

4d682e7

- Add rate variable (0.5 Hz) to detsim.conf - Add rate parameter in detsim function and MC_hits_from_files call

Modify detsim tests for new create_timestamp function

a84e91d

Update output files of test_detsim_exact and test_buffy_exact_result

Modify buffy city and source with new create timestamp function

5685a0d

MCruces-fz force-pushed the detsim_timestamp branch from d10efec to 5685a0d Compare April 7, 2021 15:31

gondiaz approved these changes Apr 7, 2021

View reviewed changes

halmamol merged commit b7ddb73 into next-exp:master Apr 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added timestamp calculation to detsim city #778

Added timestamp calculation to detsim city #778

MCruces-fz commented Mar 16, 2021

MCruces-fz commented Mar 16, 2021 •

edited

Loading

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021

MCruces-fz commented Apr 6, 2021

gondiaz commented Apr 6, 2021 •

edited

Loading

jacg commented Apr 6, 2021

MCruces-fz commented Apr 6, 2021

mmkekic commented Apr 6, 2021

MCruces-fz commented Apr 6, 2021

jacg commented Apr 6, 2021

carmenromo commented Apr 6, 2021

gondiaz commented Apr 6, 2021 •

edited

Loading

jacg commented Apr 6, 2021

carmenromo commented Apr 6, 2021

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021 •

edited

Loading

carmenromo commented Apr 6, 2021

jjgomezcadenas commented Apr 6, 2021 via email

jjgomezcadenas commented Apr 6, 2021 via email

carmenromo commented Apr 6, 2021

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021

MCruces-fz commented Apr 7, 2021

gondiaz left a comment •

edited

Loading

carmenromo commented Apr 7, 2021

jacg commented Apr 7, 2021

Added timestamp calculation to detsim city #778

Added timestamp calculation to detsim city #778

Conversation

MCruces-fz commented Mar 16, 2021

MCruces-fz commented Mar 16, 2021 • edited Loading

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021

MCruces-fz commented Apr 6, 2021

gondiaz commented Apr 6, 2021 • edited Loading

jacg commented Apr 6, 2021

MCruces-fz commented Apr 6, 2021

mmkekic commented Apr 6, 2021

MCruces-fz commented Apr 6, 2021

jacg commented Apr 6, 2021

carmenromo commented Apr 6, 2021

gondiaz commented Apr 6, 2021 • edited Loading

jacg commented Apr 6, 2021

carmenromo commented Apr 6, 2021

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021 • edited Loading

carmenromo commented Apr 6, 2021

jjgomezcadenas commented Apr 6, 2021 via email

jjgomezcadenas commented Apr 6, 2021 via email

carmenromo commented Apr 6, 2021

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021

jacg commented Apr 6, 2021

MCruces-fz commented Apr 7, 2021

gondiaz left a comment • edited Loading

Choose a reason for hiding this comment

carmenromo commented Apr 7, 2021

jacg commented Apr 7, 2021

MCruces-fz commented Mar 16, 2021 •

edited

Loading

gondiaz commented Apr 6, 2021 •

edited

Loading

gondiaz commented Apr 6, 2021 •

edited

Loading

jacg commented Apr 6, 2021 •

edited

Loading

gondiaz left a comment •

edited

Loading