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

Opacity state slice #2758

Merged
merged 17 commits into from
Aug 1, 2024
Merged

Conversation

Rodot-
Copy link
Contributor

@Rodot- Rodot- commented Jul 29, 2024

📝 Adds a geitem operation to the opacity state class which allows slicing and indexing by shell

Type: 🚀 feature
The opacity state should be able to be sliced in order to run the Montecarlo on sub-slices of the geometry. Let me know if there are other features or considerations to think about.

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

@Rodot- Rodot- marked this pull request as draft July 29, 2024 17:59
@tardis-bot
Copy link
Contributor

tardis-bot commented Jul 29, 2024

*beep* *bop*
Hi human,
I ran ruff on the latest commit (df19bae).
Here are the outputs produced.
Results can also be downloaded as artifacts here.
Summarised output:

Complete output(might be large):

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 69.35%. Comparing base (5e7a23b) to head (df19bae).
Report is 5 commits behind head on master.

Files Patch % Lines
tardis/opacities/opacity_state.py 50.00% 1 Missing ⚠️
tardis/transport/montecarlo/numba_interface.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2758      +/-   ##
==========================================
- Coverage   69.83%   69.35%   -0.48%     
==========================================
  Files         191      196       +5     
  Lines       15022    15011      -11     
==========================================
- Hits        10490    10411      -79     
- Misses       4532     4600      +68     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Rodot- Rodot- marked this pull request as ready for review July 30, 2024 14:21
Copy link
Contributor

@andrewfullard andrewfullard left a comment

Choose a reason for hiding this comment

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

I hate that this is in 2 places but oh well. Please remove the print statements from the test.

andrewfullard
andrewfullard previously approved these changes Jul 30, 2024
@@ -112,6 +112,56 @@ def __init__(
self.photo_ion_activation_idx = photo_ion_activation_idx
self.k_packet_idx = k_packet_idx

def __getitem__(self, i):
Copy link
Member

Choose a reason for hiding this comment

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

I thought we can't make new classes in a jitclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was mistaken

"""
# NOTE: This currently will not work with continuum processes since it does not slice those arrays
return OpacityState(
self.electron_density[i],
Copy link
Member

Choose a reason for hiding this comment

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

check if i is an int and then raise an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wouldn't allow one to slice the opacity state then

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, Wolfgang is concerned about what happens if somebody requests OpacityState[5] and passes that to something else.

Copy link
Member

Choose a reason for hiding this comment

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

yep. OpacityState[5] should return either a length 1 OPacity state or fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which do you prefer?

Copy link
Contributor

@andrewfullard andrewfullard left a comment

Choose a reason for hiding this comment

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

I'm happy with this.

@wkerzendorf wkerzendorf merged commit c82dde5 into tardis-sn:master Aug 1, 2024
18 checks passed
officialasishkumar added a commit to officialasishkumar/tardis that referenced this pull request Aug 2, 2024
* added setup function

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* refactor geometry calculate distances

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* Refactor montecarlo estimators in benchmark code

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* add cache and setup functions

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* add cache and setup functions

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* ran ruff linter

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* Thou hath vanquished the redundant variables and functions

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* fix bugs

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* change benchmark yml

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* repeat in class

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* Benchmark run_tardis with track_rpacket enabled (tardis-sn#2720)

* Benchmark run_tardis with track_rpacket enabled

* Update config to resemble time_run_tardis config

* Fix Benchmark error

* Cache Benchmark Property

* Remove Cached Property decorator

* rounds in benchmarks

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* fix

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* fix error in formal integral

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* more cache

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* sort by ratio

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* Donot run docs, benchmarks and stardis tests on draft PRs. (tardis-sn#2761)

* Add ready to review to run tests

* Add if condition to check PR status

* Running stardis tests only on push to master

* Removing osx-64 from workflows

* Add Extend Array Function (tardis-sn#2771)

* Add Extend Array Function

* Fix Typo and restructure

* Restructure

* Doc string update

* Was expecting numpy arrays to be passed by reference but it is not so updating it

* Add test for the extend array function

* Opacity state slice (tardis-sn#2758)

* Added basic sliing operation to opacitystate and set the transport solver to use the bounds of the gemoetry indices

* Reformatted the slicing index

* Added slicing operation with __getitem__

* Added docstring

* removed old slice method

* Ran black

* Added back the 'slice' method because __getitem__ is wacky

* added missing argument

* fixed missing argument

* ran black

* fixed the tests

* ran black

* Updated the slice to use __getitem__ again and modified the opacitystate object defined in the opacities module

* Removed old slice method and print statements

* removed print statement from the test

* added type annotation to OpacityState getitem

* updated numba interface

* Fixes black formatting

* Release Pipeline Fixes (tardis-sn#2767)

* Fix zenodo badge

* Pick from the website

* update badge in credits.rst

* Use the all versions DOI

* Inherit secrets

* Change triggers for testing

* Reset Triggers

* Fix ruff comments issue (tardis-sn#2762)

* Fix ruff comments issue

* Only check files changed

* Testing triggers

* Fix bash commands

* No pager

* fetch depth

* Reset all changes

* cache in setup function

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

* change benchmark yml

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

---------

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Co-authored-by: Sumit Gupta <53135486+Sumit112192@users.noreply.github.com>
Co-authored-by: KasukabeDefenceForce <abhinavohri13@gmail.com>
Co-authored-by: Jack O'Brien <jobrien585@gmail.com>
Co-authored-by: Andrew Fullard <andrewgfullard@gmail.com>
Co-authored-by: Atharva Arya <55894364+atharva-2001@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants