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

Basic Regression Data Setup #213

Merged
merged 18 commits into from
Oct 7, 2024

Conversation

atharva-2001
Copy link
Member

📝 Description

Type: 🚦 testing | 🎢 infrastructure

Basic regression data setup.

📌 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.

@jvshields
Copy link
Contributor

I think that this looks quite good, generally. Would using this follow the same workflow as the tardis regression tests, outlined here? https://tardis-sn.github.io/tardis/contributing/development/running_tests.html

@atharva-2001
Copy link
Member Author

Hi @jvshields, I'm afraid the docs are not up to date, but you can run tests using this --stardis-regression-data=<path to regression data>. If you use the --generate-reference flag, you can update the regression data.

@atharva-2001 atharva-2001 requested a review from jvshields October 4, 2024 08:26
Copy link
Contributor

@jvshields jvshields left a comment

Choose a reason for hiding this comment

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

Double-check the black formatting, but this looks good to me as far as I can tell.

from tardis.io.util import HDFWriterMixin


class RegressionData:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this not just inherit from the TARDIS class?

@jvshields jvshields merged commit 218678c into tardis-sn:main Oct 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants