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

Feat31 sst1 summarize bp #51

Merged
merged 23 commits into from
Oct 12, 2022
Merged

Feat31 sst1 summarize bp #51

merged 23 commits into from
Oct 12, 2022

Conversation

BijalBPatel
Copy link
Collaborator

@BijalBPatel BijalBPatel commented Oct 7, 2022

Expanded functionality of the summarize_run function in SST1RSiXSDB.py (#31). I believe the signature matches that of the old version, so existing code should remain functional.

New Features:

  • Slightly expanded set of preset keyword search terms, made many case-insensitive, made many regex-based.
  • Allowed for additional search terms to be provided as keyword arguments, specifying the match method
  • If the catalog is reduced to zero, the user is notified which search term failed to match.
  • Expanded the variety of metadata that is output to the dataframe and provided a set of preset collections of metadata through the outputType parameter, including scan numbers only
  • Allowed for additional output metadata fields to be requested through the userOutputs keyword argument
  • Implemented failing gracefully at multiple stages
  • Implemented limited 'troubleshooting tips' on bad user input

See signature docstring for full documentation and example functions.

@BijalBPatel BijalBPatel self-assigned this Oct 7, 2022
@pbeaucage pbeaucage changed the title Feat31 sst1 summarize bp Feat #31 sst1 summarize bp Oct 8, 2022
@pbeaucage pbeaucage changed the title Feat #31 sst1 summarize bp Feat31 sst1 summarize bp Oct 8, 2022
@pbeaucage
Copy link
Collaborator

Tagging issue #31 which didn't tag in title, weird. Will take a look soon.

The CI failures appear to be an unrelated issue with Python3.8 and xarray. I will open a separate issue and get those fixed.

Copy link
Collaborator

@pbeaucage pbeaucage left a comment

Choose a reason for hiding this comment

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

This is really awesome! I made a few minor code style and functionality suggestions, and a suggestion to change the method signature. Let me know what you think.

@pbeaucage
Copy link
Collaborator

pbeaucage commented Oct 8, 2022

One request that explicitly should not hold up merging: would you be willing to write a little how-to, largely adapted from the docstring, in the sphinx docs?

If you can think of any ways this method might be unit-tested it would be good to document them as well. Logistically we actually can't unit test this class until there is a public BNL Tiled server, though we could ask Dan Allan to add a rsoxs example to the public Tiled demonstration server. That wouldn't get us a rsoxs catalog, though.
update: you can actually write the test function over in tests/ and then just decorate the functions with @pytest.mark.skip(reason="cannot test until BNL Tiled is public")

@BijalBPatel BijalBPatel linked an issue Oct 9, 2022 that may be closed by this pull request
@BijalBPatel
Copy link
Collaborator Author

@BijalBPatel BijalBPatel closed this Oct 9, 2022
@BijalBPatel BijalBPatel reopened this Oct 9, 2022
@BijalBPatel
Copy link
Collaborator Author

Woops - sorry for close/opening, meant to just reply to a comment.

@pbeaucage pbeaucage self-requested a review October 12, 2022 00:09
Copy link
Collaborator

@pbeaucage pbeaucage left a comment

Choose a reason for hiding this comment

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

LGTM!

@BijalBPatel BijalBPatel merged commit 12dc6c0 into main Oct 12, 2022
@BijalBPatel BijalBPatel deleted the feat31_SST1SummarizeBP branch October 12, 2022 00:21
@BijalBPatel
Copy link
Collaborator Author

BijalBPatel commented Oct 12, 2022

One request that explicitly should not hold up merging: would you be willing to write a little how-to, largely adapted from the docstring, in the sphinx docs?

If you can think of any ways this method might be unit-tested it would be good to document them as well. Logistically we actually can't unit test this class until there is a public BNL Tiled server, though we could ask Dan Allan to add a rsoxs example to the public Tiled demonstration server. That wouldn't get us a rsoxs catalog, though.
update: you can actually write the test function over in tests/ and then just decorate the functions with @pytest.mark.skip(reason="cannot test until BNL Tiled is public")

No problem - unless someone else gets to it first, I'll take a crack at the sphinx documentation and tests in a couple of weeks.

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.

feat: SST1RSoXSDB: flexible tool for finding run numbers
2 participants