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

I/O Refactoring #2652

Merged
merged 21 commits into from
Jun 26, 2024
Merged

I/O Refactoring #2652

merged 21 commits into from
Jun 26, 2024

Conversation

andrewfullard
Copy link
Contributor

@andrewfullard andrewfullard commented Jun 10, 2024

📝 Description

Type: 🎢 infrastructure

This PR refactors the I/O part of TARDIS to split it up better by category e.g. Composition (and its parts).

Also fixes bug with CSVY dimensions

Fixes #2582

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

_from_config and _from_csvy to match typical class methods

Config first, CSVY second
With small extensions outside that area
@andrewfullard
Copy link
Contributor Author

What really needs to happen to resolve the CSVY bug (and may happen in this PR?) is to remove all loc[1:] calls across the I/O part of the code and take stock of how we really want to read in tables of densities, mass fractions, etc

Not really happy with it, but helps show some of the current problems with the config arrangement compared to the code
Also adjusts notebook to deal with quantity-based packet frequencies
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

@andrewfullard
Copy link
Contributor Author

@jvshields pluralized instances of mass_fraction in this PR. The Composition class still has lots of singular form.

jvshields
jvshields previously approved these changes Jun 24, 2024
type:
type: string
default: 'damped'
description: THIS IS A DUMMY VARIABLE DO NOT USE
Copy link
Member

Choose a reason for hiding this comment

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

are you sure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is for now until the config is restructured


Returns
-------
DiluteThermalRadiationFieldState
Copy link
Member

Choose a reason for hiding this comment

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

that needs to change at some stage to Planckian


Returns
-------
DiluteThermalRadiationFieldState
Copy link
Member

Choose a reason for hiding this comment

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

Planckian in the end.

filename or path of the density file
abundance_filetype : str
mass_fractions_filetype : str
Copy link
Member

Choose a reason for hiding this comment

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

enum?

@@ -0,0 +1,91 @@
from tardis.io.model.readers.util import read_csv_isotope_mass_fractions
Copy link
Member

Choose a reason for hiding this comment

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

do we know what's going on with this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the old reader, it actually produces a result but it's deprecated. We can clean it out once we are happy the new reader works as intended.

wkerzendorf
wkerzendorf previously approved these changes Jun 26, 2024
Copy link
Member

@wkerzendorf wkerzendorf left a comment

Choose a reason for hiding this comment

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

a few comments but mostly looks good.

@andrewfullard andrewfullard dismissed stale reviews from wkerzendorf and jvshields via 05805df June 26, 2024 17:15
@andrewfullard andrewfullard merged commit 140bb96 into tardis-sn:master Jun 26, 2024
4 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change variable name d to something more descriptive
5 participants