Arepo parser #1867

AlexHls · 2022-01-19T16:54:41Z

Parser for Arepo snapshots to csvy files.

Description

Consists of two distinct parts:

First part is a wrapper to automatically load snapshots and extract relevant data. This requires arepo-snap-util to beinstalled as an optional dependency
Second part is a converter of grid data to csvy model. Accepts any grid data as long as the shape is the same the snapshot would yield. Extracts profiles either along a line, a cone or averages over the whole star

Motivation and context

No parser available yet, need one for my own project.

How has this been tested?

Testing pipeline.
Other. I tested all functions on my own snapshots.

Examples

https://alexhls.github.io/tardis/branch/arepo-parser/io/configuration/components/models/converters/arepo_to_tardis.html

Type of change

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

Checklist

My change requires a change to the documentation.
- I have updated the documentation accordingly.
- (optional) I have built the documentation on my fork following the instructions.
I have assigned and requested two reviewers for this pull request.

review-notebook-app · 2022-01-19T16:54:45Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2022-01-19T17:09:14Z

Codecov Report

Merging #1867 (6b1b420) into master (3471658) will increase coverage by 0.39%.
The diff coverage is n/a.

❗ Current head 6b1b420 differs from pull request most recent head ce9e73c. Consider uploading reports for the commit ce9e73c to get more accurate results

@@            Coverage Diff             @@
##           master    #1867      +/-   ##
==========================================
+ Coverage   62.12%   62.52%   +0.39%     
==========================================
  Files          66       68       +2     
  Lines        6809     7124     +315     
==========================================
+ Hits         4230     4454     +224     
- Misses       2579     2670      +91

Impacted Files	Coverage Δ
.../montecarlo/montecarlo_numba/r_packet_transport.py	`16.45% <0.00%> (ø)`
tardis/tardis/io/parsers/arepo.py	`69.04% <0.00%> (ø)`
tardis/tardis/model/base.py	`88.96% <0.00%> (+0.66%)`	⬆️
...rdis/tardis/montecarlo/montecarlo_numba/vpacket.py	`19.19% <0.00%> (+0.82%)`	⬆️
.../montecarlo/montecarlo_numba/single_packet_loop.py	`31.25% <0.00%> (+1.46%)`	⬆️
...dis/tardis/montecarlo/montecarlo_numba/r_packet.py	`48.64% <0.00%> (+23.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3471658...ce9e73c. Read the comment docs.

isaacgsmith · 2022-01-20T04:21:30Z

docs/io/configuration/components/models/converters/arepo_to_tardis.ipynb

@@ -0,0 +1,220 @@
+{


Perhaps a shorter title. Also, you should start the notebook with some information about what Arepo is, what a snapshot is, and link Arepo's documentation/website.

Reply via ReviewNB

isaacgsmith · 2022-01-20T04:21:30Z

docs/io/configuration/components/models/converters/arepo_to_tardis.ipynb

@@ -0,0 +1,220 @@
+{


You should explain what those two files are. One other suggestion: instead of using try/except, you could just put the code in a markdown codeblock and explain that it shows how to take the snapshot if you have the package installed.

Reply via ReviewNB

isaacgsmith · 2022-01-20T04:21:30Z

docs/io/configuration/components/models/converters/arepo_to_tardis.ipynb

@@ -0,0 +1,220 @@
+{


I would explain what is included in the JSON file/how you get the file.

Also, see if you can link the specific section of the API, or just summarize the options (or use the python help function in the notebook, any of those work)

Reply via ReviewNB

isaacgsmith · 2022-01-20T04:21:30Z

docs/io/configuration/components/models/converters/arepo_to_tardis.ipynb

@@ -0,0 +1,220 @@
+{


Could you hyperlink the section on the CSVY model?

Reply via ReviewNB

isaacgsmith · 2022-01-20T04:21:30Z

docs/io/configuration/components/models/converters/arepo_to_tardis.ipynb

@@ -0,0 +1,220 @@
+{


I think that last line is important enough to create a note box

Reply via ReviewNB

tardis-bot · 2022-01-21T10:03:35Z

Before a pull request is accepted, it must meet the following criteria:

Is the necessary information provided?
Is this a duplicate PR?
- If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
- If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
Does it pass existing tests and are new tests provided if required?
- The test coverage should not decrease, and for new features should be close to 100%.
Is the code tidy?
- No unnecessary print lines or code comments.

tardis/io/parsers/arepo.py

andrewfullard · 2022-01-21T16:45:13Z

tardis/io/tests/data/arepo_line_reference_model.csvy

+nan,nan,nan,nan
+nan,nan,nan,nan
+nan,nan,nan,nan
+nan,nan,nan,nan


Why are these NaN?

When rebinning data with scipy.stats.binned_statistic to a higher resolution, it will insert bins with NaN. This behaviour is intended/ I did not include an error for this since it does not break the export and can be adressed manually later on.

isaacgsmith · 2022-01-21T19:57:55Z

docs/io/configuration/components/models/converters/arepo_to_tardis.ipynb

@@ -0,0 +1,364 @@
+{


This should link to io/configuration/components/models/index.rst#csvy-model

Reply via ReviewNB

isaacgsmith · 2022-01-21T19:57:55Z

docs/io/configuration/components/models/converters/arepo_to_tardis.ipynb

@@ -0,0 +1,364 @@
+{


While this works I would recommend using:

<div class="alert alert-info">

Note

insert text of note here

</div>

in a markdown cell (same for the other notes).

Reply via ReviewNB

isaacgsmith · 2022-01-21T19:57:55Z

docs/io/configuration/components/models/converters/arepo_to_tardis.ipynb

@@ -0,0 +1,364 @@
+{


I'd still mention that arepo_snapshot.hdf5 is the output file of arepo.

Reply via ReviewNB

isaacgsmith · 2022-01-21T19:57:55Z

docs/io/configuration/components/models/converters/arepo_to_tardis.ipynb

@@ -0,0 +1,364 @@
+{


Do users have to generate the JSON themselves? Or can they get it from arepo? Seems like in either case the user would need the package?

Reply via ReviewNB

Yes, the JSON files have to be generated by the user themselves. It is really only a shortcut to having the data available for the documentation, as getting it directly from the snapshot would introduce the arepo-snap-util package as a dependency. (This is not really advisable for two reasons: a) the package contains lots of unnecessary other utilties and the reelevant parts are mostly written in c anyway. b) It is as far as I know not publicly available.)

Rewriting the snapshot reader in Python is really not feasible; besides, this PR is written for the development version anyways, which is also not publicly available. And I recon that most people who use the development version also have the arepo-snap-util package. The public version should in principle also work, but I have no idea how these snapshots are read out. Here the users can save the data to JSON files and use the parser this way. But these files have to be generated by the user, because this is intended as a fallback for exotic Arepo snapshots anyway.

andrewfullard

Approved, will merge once @smithis7 comments are addressed

isaacgsmith · 2022-01-26T06:11:53Z

docs/io/configuration/components/models/converters/arepo_to_tardis.ipynb

@@ -0,0 +1,395 @@
+{


Very last thing. In the parentheses it should say (see CSVY Model). The link is correct, just the hyperlink text does not match the link.

Reply via ReviewNB

isaacgsmith

Docs are approved

AlexHls added 7 commits January 19, 2022 10:42

Initial commit. Basic line profile

32576a5

Added documentation sketch

286337c

Fixed nuclear abundance readout

1bcf19d

[build docs]

bab77a4

Allowed errors in documentation notebook [build docs]

75fa638

Fixed documentation notebook [build docs]

930cd6d

Fixed documentation notebook [build docs]

06cfd4b

isaacgsmith reviewed Jan 20, 2022

View reviewed changes

AlexHls added 6 commits January 20, 2022 17:05

Added more profile options, CLI. Improved documentation [build docs]

4e3290d

Fixed wrong import in documentation [build docs]

47c5898

Fixed wrong import in documentation tardis-sn#2 [build docs]

b97a50b

Cleaned up documentation [build docs]

69fd926

Cleaned up documentation [build docs]

1d6dfc4

Fixed plotting error in documentation [build docs]

818d29d

AlexHls marked this pull request as ready for review January 21, 2022 09:44

AlexHls added documentation io labels Jan 21, 2022

AlexHls requested a review from andrewfullard January 21, 2022 10:03

AlexHls added 6 commits January 21, 2022 16:24

Added tests [build docs]

4e2463c

Fixed formatting of test [build docs]

4c167df

[build docs]

d2002b4

Added name to .mailmap

e48f027

Added name to .mailmap tardis-sn#2

587489c

[build docs]

0cdf03b

andrewfullard reviewed Jan 21, 2022

View reviewed changes

isaacgsmith reviewed Jan 21, 2022

View reviewed changes

Fixed typos in docstring

2bd702d

Added propper links to documentation [build docs]

51972d4

AlexHls requested review from isaacgsmith and andrewfullard January 24, 2022 12:14

andrewfullard approved these changes Jan 24, 2022

View reviewed changes

wkerzendorf requested a review from afloers January 24, 2022 16:20

isaacgsmith reviewed Jan 26, 2022

View reviewed changes

Fixed hyperlink text in documentation [build docs]

ce9e73c

AlexHls requested a review from isaacgsmith January 26, 2022 09:13

isaacgsmith approved these changes Jan 26, 2022

View reviewed changes

wkerzendorf merged commit bb4304b into tardis-sn:master Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arepo parser #1867

Arepo parser #1867

AlexHls commented Jan 19, 2022 •

edited

Loading

review-notebook-app bot commented Jan 19, 2022

codecov bot commented Jan 19, 2022 •

edited

Loading

isaacgsmith Jan 20, 2022 •

edited

Loading

isaacgsmith Jan 20, 2022 •

edited

Loading

isaacgsmith Jan 20, 2022

isaacgsmith Jan 20, 2022 •

edited

Loading

isaacgsmith Jan 20, 2022 •

edited

Loading

tardis-bot commented Jan 21, 2022

andrewfullard Jan 21, 2022

AlexHls Jan 21, 2022

isaacgsmith Jan 21, 2022 •

edited

Loading

isaacgsmith Jan 21, 2022 •

edited

Loading

isaacgsmith Jan 21, 2022 •

edited

Loading

isaacgsmith Jan 21, 2022 •

edited

Loading

AlexHls Jan 24, 2022

andrewfullard left a comment

isaacgsmith Jan 26, 2022 •

edited

Loading

isaacgsmith left a comment

Arepo parser #1867

Arepo parser #1867

Conversation

AlexHls commented Jan 19, 2022 • edited Loading

review-notebook-app bot commented Jan 19, 2022

codecov bot commented Jan 19, 2022 • edited Loading

Codecov Report

isaacgsmith Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

isaacgsmith Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

isaacgsmith Jan 20, 2022

Choose a reason for hiding this comment

isaacgsmith Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

isaacgsmith Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

tardis-bot commented Jan 21, 2022

andrewfullard Jan 21, 2022

Choose a reason for hiding this comment

AlexHls Jan 21, 2022

Choose a reason for hiding this comment

isaacgsmith Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

isaacgsmith Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

isaacgsmith Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

isaacgsmith Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

AlexHls Jan 24, 2022

Choose a reason for hiding this comment

andrewfullard left a comment

Choose a reason for hiding this comment

isaacgsmith Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

isaacgsmith left a comment

Choose a reason for hiding this comment

AlexHls commented Jan 19, 2022 •

edited

Loading

codecov bot commented Jan 19, 2022 •

edited

Loading

isaacgsmith Jan 20, 2022 •

edited

Loading

isaacgsmith Jan 20, 2022 •

edited

Loading

isaacgsmith Jan 20, 2022 •

edited

Loading

isaacgsmith Jan 20, 2022 •

edited

Loading

isaacgsmith Jan 21, 2022 •

edited

Loading

isaacgsmith Jan 21, 2022 •

edited

Loading

isaacgsmith Jan 21, 2022 •

edited

Loading

isaacgsmith Jan 21, 2022 •

edited

Loading

isaacgsmith Jan 26, 2022 •

edited

Loading