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

Improved arepo parser #1941

Merged
merged 19 commits into from
Jul 12, 2022
Merged

Improved arepo parser #1941

merged 19 commits into from
Jul 12, 2022

Conversation

AlexHls
Copy link
Member

@AlexHls AlexHls commented Mar 31, 2022

Performance improvements to the arepo parser + deprecation of line profiles.

Description

Currently the arepo parser maps snapshots to a Cartesian grid before profile extraction. This is extremely inefficient/ compute time intensive. The mapping is only needed for the line profile. Since line profiles can also be achieved by a very narrow cone, line profiles will be deprecated as well as the mapping to Cartesian grids. Profiles will be extracted directly from the snapshot cells.

Motivation and context

Current implementation is extremely inefficient and contains unneeded mapping procedure.

How has this been tested?

  • Testing pipeline.
  • Other.

Examples

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • Performance improvement.

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
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@AlexHls AlexHls requested a review from Rodot- July 8, 2022 19:29
@AlexHls
Copy link
Member Author

AlexHls commented Jul 11, 2022

/azp run compare-refdata

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 1941 in repo tardis-sn/tardis

@andrewfullard
Copy link
Contributor

/azp run compare-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Build failed 4b7cb2b

Click here to see results.

@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #1941 (4b198ed) into master (a57ab47) will increase coverage by 0.09%.
The diff coverage is 2.63%.

❗ Current head 4b198ed differs from pull request most recent head 2a49031. Consider uploading reports for the commit 2a49031 to get more accurate results

@@            Coverage Diff             @@
##           master    #1941      +/-   ##
==========================================
+ Coverage   58.04%   58.13%   +0.09%     
==========================================
  Files          76       75       -1     
  Lines        8743     8602     -141     
==========================================
- Hits         5075     5001      -74     
+ Misses       3668     3601      -67     
Impacted Files Coverage Δ
tardis/io/parsers/arepo.py 66.35% <0.00%> (-2.70%) ⬇️
.../montecarlo/montecarlo_numba/single_packet_loop.py 25.75% <0.00%> (-1.11%) ⬇️
tardis/transport/r_packet_transport.py 17.97% <3.44%> (-2.03%) ⬇️

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

@AlexHls AlexHls marked this pull request as ready for review July 11, 2022 20:19
@AlexHls AlexHls requested a review from andrewfullard July 11, 2022 20:20
@Rodot- Rodot- merged commit e01e44a into tardis-sn:master Jul 12, 2022
@AlexHls AlexHls deleted the arepo-parser-optimized branch July 12, 2022 13:17
jayantbhakar pushed a commit to jayantbhakar/tardis that referenced this pull request Jul 29, 2022
* Removed line profile as option

* Removed mapping to Cartesian grid

* Removed automatic plotting after profile creation

* Fixed typo in arepo plot

* Updated documentation

* [build docs]

* [build docs] Removed outputs from doc-notebook

* Updated tests with new models

* Removed deprecated keyword in snapshot loader

* Replaced wget with requests for testfile download

* Updated file retrieval for testing

* Fixed fixture request

* Added arepo data to refdata helper

* Updated refdata location in docs

* Updated paths in tests

* Update download_reference_data.sh

* Testing areop_parser_tests

* Fixed deleted reference file
andrewfullard pushed a commit that referenced this pull request Aug 3, 2022
* rpacket_tracker returns dataframe

* added black formatting

* replaced variable i

* added rpacket_tracker_df as separate variable

* added preallocation step

* added non-preallocated-df generator function

* tests for df generator function

* renamed variables

* assert statements corrected

* added seed column in df

* added documentation for rpacket_tracker_df

* added doc str

* initial commit

* updated documention

* moved function to r_packet.py

* added block indexing to df

Co-authored-by: Wolfgang Kerzendorf <wkerzendorf@gmail.com>

* updated the df function

* added multiindexing

* shifted the df function call

* initial commit

* interaction type added

* removed unused imports

* added tests

* initial commit

* interaction type added

* Created a smart HDF Reader which can re-produce simulation object (#2052)

* Added functions to store and load a runner object

* Updated hardcode file name in runner_from_hdf

* Updated store_runner_to_hdf to append to file

* Added name and email to .mailmap

* runner hdf functions convert to cgs

* store_runner_to_hdf clears group before storing

* Storing units for astropy quantities

* Formatted code with black

* Added functions to store and load model objects

* Fixed typo and decoded string from bytes

* Fixed typo

* Added test for model_to_dict

* Formatted previous test code

* Added test for store_model_to_hdf

* Fixed formatting of model_to_hdf

* Fixed issue with test for model_to_dict

* Added test for runner_to_dict

* Fixed issue with single packet seed storage

* Updated test null value of single packet seed

* Added test for store_runner_to_hdf

* Updated to use isinstance and hasattr

* runner_to_dict returns dicts instead of iterators

* Reordered imports to match pep8 style

* Remove yaml_load_config_file (#2062)

* Remove yaml_load_config_file

* updated mailmap

* Updated __init__.py

* Modified Function Name in Config validator according to PEP8 (#2076)

Fix function name according pep8

* Added documetation on how to compare environment before update (#2082)

* Moved `trace_packet` to tardis tansport (#2083)

* Moved the trace_packet function to the transport module, renamed the function to remove continuum reference as it is generic

* Formatted to PEP 8

* Reformatted single_packet_loop for PEP 8

* removed old continuum trace packet files

* removed references to old files

* Added some docstrings

* Missed an old function call

* Fix broken links in workflow files and goverance md (#2084)

* Adding physics introduction to documentation (#2026)

* creating files and sections

* adding images

* writing intro to tardis physics and adding it to sidebar

* changing spectra page structure

* mostly complete draft of light and matter page

* spectrum page

* fixing up physics stuff

* fixing docstring

* Revert "fixing docstring"

This reverts commit e24cadd.

* andrew's changes

* a few more edits

* adding note about angstroms

* Improved arepo parser (#1941)

* Removed line profile as option

* Removed mapping to Cartesian grid

* Removed automatic plotting after profile creation

* Fixed typo in arepo plot

* Updated documentation

* [build docs]

* [build docs] Removed outputs from doc-notebook

* Updated tests with new models

* Removed deprecated keyword in snapshot loader

* Replaced wget with requests for testfile download

* Updated file retrieval for testing

* Fixed fixture request

* Added arepo data to refdata helper

* Updated refdata location in docs

* Updated paths in tests

* Update download_reference_data.sh

* Testing areop_parser_tests

* Fixed deleted reference file

* Arepo docs fix (#2088)

* [build-docs] Fixed link in arepo docs

* [build-docs] Changed filename in arepo docs

* [build-docs]

* [build-docs]

* [build-docs]

* [build docs]

* [build-docs] Changed refdata url

* [build_docs]

* Updated mailmap

* [build_docs] Updated url for refdata download

* Creating new docs building instructions (#2097)

* creating new docs building instructions

* typos, grammar, and text in hyperlinks

* added tests

* added documentation, black formatting

Co-authored-by: Wolfgang Kerzendorf <wkerzendorf@gmail.com>
Co-authored-by: Satwik Kambham <satwik.kambham@gmail.com>
Co-authored-by: aman kumar <ak584584@gmail.com>
Co-authored-by: Rohith Varma Buddaraju <rohith.varma.buddaraju@gmail.com>
Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>
Co-authored-by: Jack O'Brien <jobrien585@gmail.com>
Co-authored-by: Isaac Smith <71480393+smithis7@users.noreply.github.com>
Co-authored-by: AlexHls <70367168+AlexHls@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants