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

pspecdata input calibration and pspec_run updates #212

Merged
merged 14 commits into from
Jul 25, 2019
Merged

Conversation

nkern
Copy link
Member

@nkern nkern commented Jul 4, 2019

General updates in support of H1C_IDR2 pipeline overhaul. This includes:

• allows PSpecData.add() to take a calibration file and apply on-the-fly

• new interleave_times kwarg to pspec_run

• allow for _load_dsets to take multiple files for a single entry in dsets

pspec_run now only returns ds instead of psc, ds

ds.validate_datasets bug fix when doing auto-dset power spectra

• optional SWMR mode in PSpecContainer

• minor changes to grouping.bootstrap_run to incorporate PSpecContainer transactional mode

@nkern nkern requested a review from philbull July 4, 2019 06:27
nkern added 3 commits July 4, 2019 12:08
	modified:   pspecdata.py
	modified:   tests/test_pspecdata.py
	modified:   utils.py
@nkern nkern changed the title pspecdata input calibration pspecdata input calibration and pspec_run updates Jul 9, 2019
@philbull philbull self-assigned this Jul 9, 2019
@coveralls
Copy link

coveralls commented Jul 10, 2019

Coverage Status

Coverage increased (+0.02%) to 96.68% when pulling 8eff919 on pspecdata_apply_cal into aca6586 on master.

@nkern nkern requested a review from plaplant July 11, 2019 00:40
@nkern
Copy link
Member Author

nkern commented Jul 11, 2019

@plaplant okay I added a print warning and updated a docstring in PSpecContainer in container.py. Let me know if you have any further comments on SWMR usage in PSpecContainer (no need to review the whole PR if you are pressed for time) thanks!

@plaplant
Copy link
Member

@nkern I think the way forward here is to run a check each time a new group or dataset would be added to a pspec file, and raise an error if the file is in SWMR mode. I agree that I don't want to hamstring the user too much, but also want to make sure that we're using the library correctly. (Re: the issue you posted yesterday, the library as currently written seems to be able to create datasets without error, but I am skeptical the data are actually being written correctly. Even if you showed it was not writing nonsense for a few datasets, I don't think we should be doing it at all because the library makes no guarantees to correctness, and places all responsibility on the user.)

I think defaulting to swmr=False (as I think you've done here) and having the aforementioned check would be sufficient to make things "safe enough" for the typical user. Thanks for adding the documentation, that's also very helpful for the user to understand the limitations. I can take a look, but probably can't get to it for another ~week or two.

@nkern
Copy link
Member Author

nkern commented Jul 11, 2019

@philbull @plaplant Okay, I've updated PspecContainer such that it will not write new groups or datasets if SWMR is turned on. This necessitated changing some of the testing structure in test_pspecontainer.py, but I think everything is consistent now. @philbull feel free to review the PR now and @plaplant let me know if you have any more concerns on SWMR usage in PSpecContainer (again no need to review the whole PR, Paul). Thanks!

Copy link
Collaborator

@philbull philbull left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor comments on docstrings and a query about whether to move some utility functions.

hera_pspec/grouping.py Outdated Show resolved Hide resolved
hera_pspec/grouping.py Outdated Show resolved Hide resolved
hera_pspec/pspecdata.py Outdated Show resolved Hide resolved
hera_pspec/pspecdata.py Show resolved Hide resolved
hera_pspec/pspecdata.py Show resolved Hide resolved
hera_pspec/pspecdata.py Show resolved Hide resolved
@nkern nkern merged commit 58d860b into master Jul 25, 2019
@nkern nkern deleted the pspecdata_apply_cal branch July 25, 2019 17:04
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.

None yet

4 participants