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

Removed the deprecated --with-vpacket-logging flag #1459

Merged
merged 8 commits into from
Mar 11, 2021
Merged

Removed the deprecated --with-vpacket-logging flag #1459

merged 8 commits into from
Mar 11, 2021

Conversation

antreev-brar
Copy link
Contributor

@antreev-brar antreev-brar commented Feb 14, 2021

This Pull request aims to resolve the issue #1342 Remove virtual packet logging compile flag as it was deprecated with the numba code.

Description

The changes made to the code are

  • Removed add_command_option for the with-vpacket-logging in setup.py
  • Removed a comment that described how to use full_hdf_properties with with-vpacket-logging flag in tardis/montecarlo/base.py
  • Updated warning on properties virtual_packet_nu , virtual_packet_energy, virtual_packet_luminosity that worked with with-vpacket-logging flag in tardis/montecarlo/base.py

Motivation and Context

This pull request addresses the issue raised in #1342
Earlier there were several essential features of tardis that require compiling it with vpacket logging, but this was changed with numba-montecarlo as described in issue #1215 . I went through the code and removed instances of "with-vpacket-logging". I was unsure whether the properties must be removed but I ensured that they weren't called from anywhere in the code. Hence they were redundant as well.

How Has This Been Tested?

  • Testing pipeline
  • Reference Data Comparison following these instructions
  • Other (please describe)

I checked it by running unit tests on it with python setup.py test that gave the following output on my local machine.

  • 156 passed, 1254 skipped, 6 xpassed in 7.87s

I also ran python setup.py test --args="--tardis-refdata=/path/to/tardis-refdata/" test and gave the following log output

  • 19 passed in 2.72s

Also the example notebook given in quickstart https://tardis-sn.github.io/tardis/quickstart/quickstart.html executed without any issues.

Screenshots (if appropriate):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • None of the above (please describe)

Removed deprecated segment from the code.

Checklist:

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have built the documentation on my fork following these instructions
  • I have assigned and requested two reviewers for this pull request

@codecov
Copy link

codecov bot commented Feb 14, 2021

Codecov Report

Merging #1459 (f377b8c) into master (50f13ba) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1459   +/-   ##
=======================================
  Coverage   71.13%   71.13%           
=======================================
  Files          67       67           
  Lines        5523     5523           
=======================================
  Hits         3929     3929           
  Misses       1594     1594           
Impacted Files Coverage Δ
tardis/tardis/io/util.py 72.09% <0.00%> (ø)
tardis/tardis/montecarlo/base.py 86.20% <0.00%> (ø)

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 50f13ba...730c20f. Read the comment docs.

@antreev-brar
Copy link
Contributor Author

antreev-brar commented Feb 14, 2021

@wkerzendorf can you review this PR?

tardis/montecarlo/base.py Outdated Show resolved Hide resolved
@wkerzendorf
Copy link
Member

@antreev-brar There is additional vpacket code fragments left at https://github.com/tardis-sn/tardis/blob/master/tardis/montecarlo/setup_package.py

@antreev-brar
Copy link
Contributor Author

@wkerzendorf Thanks for letting me know. From my point of view, the code fragment uses the "build", "install", and "develop" parameters of "with-vpacket-logging" from the setup.py ( which were already removed ). Therefore we should remove this code fragment as well.

I have changed this file.

@andrewfullard andrewfullard merged commit 5b98c40 into tardis-sn:master Mar 11, 2021
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
* numba_warning

* removed--with-vpacket-logging

* Revised warnings

* removed_vpacket_code_fragment

Co-authored-by: antreev-brar <[email protected]>
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.

Remove virtual packet logging compile flag
3 participants