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

REF_M: Add the capability to autoreduce two samples from the same run #166

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

jmborr
Copy link
Member

@jmborr jmborr commented Jun 24, 2024

References EWM 5797

Short description of the changes:

Update the reduction options form for instrument REF_M with entries for up to three samples.

Check list for the pull request

  • I have updated tests for my changes

Check list for the reviewer

  • I have read the [CONTRIBUTING]
  • I have verified the proposed changes
  • best software practices
    • all internal functions have an underbar, as is python standard
    • clearly named variables (better to be verbose in variable names)
    • code comments explaining the intent of code blocks
  • All the tests are passing
  • The documentation is up to date
  • code comments added when explaining intent

Manual test for the reviewer

Run unit test test_ref_m but first remove comment to print the HTML form. After that point your browser to file:///tmp/junk.html to see the form with entries for three samples.

Signed-off-by: Jose Borreguero <[email protected]>
@jmborr jmborr self-assigned this Jun 24, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.16%. Comparing base (70d4c67) to head (2328f69).

Additional details and impacted files
@@            Coverage Diff             @@
##             next     #166      +/-   ##
==========================================
+ Coverage   77.07%   77.16%   +0.08%     
==========================================
  Files          50       50              
  Lines        4616     4634      +18     
==========================================
+ Hits         3558     3576      +18     
  Misses       1058     1058              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Jose Borreguero <[email protected]>
@jmborr jmborr force-pushed the refm_multi_samples branch from a27a83a to cd936e0 Compare June 24, 2024 19:40
@jmborr jmborr requested a review from backmari June 25, 2024 14:32
backmari
backmari previously approved these changes Jun 25, 2024
Copy link
Contributor

@backmari backmari left a comment

Choose a reason for hiding this comment

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

I ran the manual test and could see the REF_M configuration form with parameters added to accommodate up to three samples according to the requirements by the instrument scientists. Unit tests have been modified to verify the new form fields. Looks good to me.

image

Copy link
Contributor

@backmari backmari left a comment

Choose a reason for hiding this comment

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

The manual test worked as expected, and removing the unused form field seems reasonable.

image

Copy link
Contributor

@backmari backmari left a comment

Choose a reason for hiding this comment

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

removed duplicate review

@jmborr jmborr merged commit a45c070 into next Aug 6, 2024
4 checks passed
@jmborr jmborr deleted the refm_multi_samples branch August 6, 2024 19:09
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.

3 participants