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

Added compatability to current version of carsus atomic data #2019

Merged
merged 17 commits into from May 18, 2022

Conversation

Rodot-
Copy link
Contributor

@Rodot- Rodot- commented May 9, 2022

Changed the way that the atomic data objects are instantiated in TARDIS to handle atomic data produced by carsus

Motivation and context
Currently, carsus and tardis are incompatible when using atomic data containing collisional data.

How has this been tested?
Local tests with current carsus branch

  • Testing pipeline
  • Other

Examples

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • None of the above

Checklist

  • I have updated the documentation according to my changes
  • I have built the documentation by applying the build_docs label to this pull request (if you don't have enough privileges a reviewer will do it for you)
  • I have requested two reviewers for this pull request

@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #2019 (f605df3) into master (c188b8a) will decrease coverage by 0.02%.
The diff coverage is 24.13%.

@@            Coverage Diff             @@
##           master    #2019      +/-   ##
==========================================
- Coverage   60.04%   60.01%   -0.03%     
==========================================
  Files          70       70              
  Lines        8121     8133      +12     
==========================================
+ Hits         4876     4881       +5     
- Misses       3245     3252       +7     
Impacted Files Coverage Δ
tardis/plasma/properties/atomic.py 55.73% <0.00%> (-0.45%) ⬇️
tardis/plasma/properties/continuum_processes.py 35.75% <0.00%> (ø)
tardis/io/atom_data/base.py 81.81% <28.00%> (-7.90%) ⬇️
tardis/io/atom_data/atom_web_download.py 40.00% <0.00%> (-2.11%) ⬇️
tardis/io/util.py 78.26% <0.00%> (+6.26%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Rodot- Rodot- added plasma atomic bugfix 🪲 opacity Related to opacity extensions in TARDIS labels May 9, 2022
@Rodot-
Copy link
Contributor Author

Rodot- commented May 10, 2022

Should be noted, this branch now seems to work but it must be run single-threaded.

@Rodot-
Copy link
Contributor Author

Rodot- commented May 10, 2022

We should start thinking about writing tests, but that could go into a later PR, what does everyone think?

@epassaro
Copy link
Member

epassaro commented May 10, 2022

We should start thinking about writing tests, but that could go into a later PR, what does everyone think?

We are currently adding more metadata and versioning Carsus format, so the key checkingássigning must be handled differently. I hope we have this ready for Thursday.

EDIT: I mean, after these new changes the new Carsus files are going to be signed with "format_version: v1.0" and old atomic files must be identified as "legacy" or something like that in TARDIS.

@Rodot- Rodot- mentioned this pull request May 11, 2022
10 tasks
@Rodot- Rodot- marked this pull request as ready for review May 16, 2022 16:03
@Rodot- Rodot- requested a review from andrewfullard May 16, 2022 20:22
@Rodot- Rodot- requested a review from sonachitchyan May 16, 2022 20:46
@epassaro
Copy link
Member

Is this ready? @sonachitchyan @Rodot-

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

@epassaro
Copy link
Member

How has this been tested? It hasn't really

We can say this has been tested against old Carsus atomic files.

New atomic files were tested locally, right?

@Rodot-
Copy link
Contributor Author

Rodot- commented May 16, 2022

Yep, we tested the new ones locally and the old ones are tested by the pipeline

… version. This commit also adds credit to all of the people who helped out building this PR who are not already marked as contributors

Co-authored-by: Sona Chitchyan <chitchyan.sona@gmail.com>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Ezequiel Pássaro <epassaro15@gmail.com>
Co-authored-by: Laureano Martinez <laureanomartinez1234@gmail.com>
@Rodot-
Copy link
Contributor Author

Rodot- commented May 18, 2022

@epassaro @andrewfullard @chvogl I think this is ready to merge if you add your approvals, unless anyone has any final comments.

tardis/io/atom_data/base.py Outdated Show resolved Hide resolved
@Rodot- Rodot- merged commit f27fa30 into tardis-sn:master May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
atomic bugfix 🪲 opacity Related to opacity extensions in TARDIS plasma
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants