Skip to content

NMR and XRD block improvements #1119

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

Merged
merged 7 commits into from
Apr 18, 2025
Merged

NMR and XRD block improvements #1119

merged 7 commits into from
Apr 18, 2025

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Apr 12, 2025

Main one being using a temp dir when extracting data, to avoid clogging up the server.

Also improve the NMR block plots and nest metadata under their own key (which the app will now try to use by default in the UI, falling back to the original).

@ml-evs ml-evs added the datablock An issue pertaining to a specific datablock label Apr 12, 2025
Copy link

codecov bot commented Apr 12, 2025

Codecov Report

Attention: Patch coverage is 82.43243% with 13 lines in your changes missing coverage. Please review.

Project coverage is 71.30%. Comparing base (39a8a70) to head (0568f3f).
Report is 110 commits behind head on main.

Files with missing lines Patch % Lines
pydatalab/src/pydatalab/apps/nmr/blocks.py 80.76% 10 Missing ⚠️
pydatalab/src/pydatalab/apps/xrd/utils.py 83.33% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1119      +/-   ##
==========================================
+ Coverage   70.28%   71.30%   +1.01%     
==========================================
  Files          63       63              
  Lines        4133     4144      +11     
==========================================
+ Hits         2905     2955      +50     
+ Misses       1228     1189      -39     
Files with missing lines Coverage Δ
pydatalab/src/pydatalab/apps/nmr/utils.py 37.80% <100.00%> (+0.90%) ⬆️
pydatalab/src/pydatalab/apps/xrd/utils.py 56.79% <83.33%> (+0.54%) ⬆️
pydatalab/src/pydatalab/apps/nmr/blocks.py 86.25% <80.76%> (+56.83%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

cypress bot commented Apr 12, 2025

datalab    Run #3128

Run Properties:  status check passed Passed #3128  •  git commit 2bedbdb2a2 ℹ️: Merge 0568f3f0f534a1c5b862767aeab1e9ae193b580f into 39a8a70f14165423d24fe4f48e02...
Project datalab
Branch Review ml-evs/use-tmp-for-rasx
Run status status check passed Passed #3128
Run duration 07m 19s
Commit git commit 2bedbdb2a2 ℹ️: Merge 0568f3f0f534a1c5b862767aeab1e9ae193b580f into 39a8a70f14165423d24fe4f48e02...
Committer Matthew Evans
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 477
View all changes introduced in this branch ↗︎

@ml-evs ml-evs requested a review from be-smith April 12, 2025 14:14
@ml-evs
Copy link
Member Author

ml-evs commented Apr 12, 2025

Probably one for @jdbocarsly and @be-smith to review when you get a moment

be-smith
be-smith previously approved these changes Apr 15, 2025
Copy link
Contributor

@be-smith be-smith left a comment

Choose a reason for hiding this comment

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

XRD works fine! Had a look at NMR and seems good but am less familiar with the code. Both plotted example data succesfully

@ml-evs ml-evs dismissed stale reviews from BenjaminCharmes and be-smith via 1bbf3d5 April 15, 2025 18:15
@ml-evs ml-evs force-pushed the ml-evs/use-tmp-for-rasx branch 3 times, most recently from e2d0edf to b7ff5eb Compare April 16, 2025 14:53
@ml-evs
Copy link
Member Author

ml-evs commented Apr 16, 2025

Made a couple of tweaks to testing since your reviews @be-smith and @BenjaminCharmes, will also wait for @jdbocarsly sign off anyway

Copy link
Contributor

@BenjaminCharmes BenjaminCharmes left a comment

Choose a reason for hiding this comment

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

Everything still works well!

Copy link
Member

@jdbocarsly jdbocarsly 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, see comment on third column of the rasx format

@ml-evs ml-evs force-pushed the ml-evs/use-tmp-for-rasx branch from b7ff5eb to 0568f3f Compare April 18, 2025 08:29
@ml-evs ml-evs added the refactoring For issues/PRs that refactor existing code/features label Apr 18, 2025
@ml-evs ml-evs enabled auto-merge (rebase) April 18, 2025 08:48
@ml-evs ml-evs merged commit e5c09d9 into main Apr 18, 2025
17 checks passed
@ml-evs ml-evs deleted the ml-evs/use-tmp-for-rasx branch April 18, 2025 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datablock An issue pertaining to a specific datablock refactoring For issues/PRs that refactor existing code/features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants