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

Reference Data Generation Fixes #2562

Merged

Conversation

atharva-2001
Copy link
Member

📝 Description

Type: 🪲 bugfix 🚦 testing |

  • change hdf properties for simulation.transport

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • My changes can't be tested (explain why)

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 68.17%. Comparing base (1718002) to head (43cfa63).
Report is 6 commits behind head on master.

❗ Current head 43cfa63 differs from pull request most recent head 9dd7c85. Consider uploading reports for the commit 9dd7c85 to get more accurate results

Files Patch % Lines
tardis/simulation/tests/test_simulation.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2562      +/-   ##
==========================================
- Coverage   68.18%   68.17%   -0.01%     
==========================================
  Files         168      168              
  Lines       14219    14223       +4     
==========================================
+ Hits         9695     9697       +2     
- Misses       4524     4526       +2     

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

@atharva-2001
Copy link
Member Author

/compare-refdata

@atharva-2001
Copy link
Member Author

/update-refdata

@tardis-bot
Copy link
Contributor

tardis-bot commented Mar 27, 2024

*beep* *bop*

Hi, human.

The update-refdata workflow has failed

Click here to see the build log.

@atharva-2001
Copy link
Member Author

/update-refdata

@atharva-2001 atharva-2001 force-pushed the simulation_transport_hdf_gen_fix branch from 40d8388 to dc2cd8f Compare April 1, 2024 13:30
@andrewfullard andrewfullard self-requested a review April 1, 2024 14:08
@atharva-2001 atharva-2001 marked this pull request as draft April 2, 2024 11:02
@atharva-2001 atharva-2001 marked this pull request as ready for review April 3, 2024 09:25
@andrewfullard
Copy link
Contributor

Can you explain the test failures?

@atharva-2001
Copy link
Member Author

It's because the reference data needs to be updated. The update refdata workflow will not work on this PR because the YAML file on the master does not have steps which add the regression tests settings. This PR fixes that. Once this PR is in, we can either run the workflow or I can update the repo and the tests should work then.

@andrewfullard
Copy link
Contributor

This is simply a change in where the reference data are located in the HDF file, and not a change in the numbers, right?

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
@atharva-2001
Copy link
Member Author

This is simply a change in where the reference data are located in the HDF file, and not a change in the numbers, right?

No, I don't think there is a change in numbers.

@andrewfullard
Copy link
Contributor

I would like to get this merged ASAP, but it would be best after #2119 because that changes quite a few files.

@andrewfullard
Copy link
Contributor

After rebasing #2119 it actually changes only a few files (good!) so this should be mergeable now?

@andrewfullard andrewfullard self-requested a review April 22, 2024 14:20
@andrewfullard andrewfullard merged commit 7befaa0 into tardis-sn:master Apr 22, 2024
6 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants