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

spatial_hist is a class instance #271

Open
josh0-jrg opened this issue Nov 2, 2022 · 6 comments
Open

spatial_hist is a class instance #271

josh0-jrg opened this issue Nov 2, 2022 · 6 comments

Comments

@josh0-jrg
Copy link

josh0-jrg commented Nov 2, 2022

When using nestSpatialRateNRSource or standard flamedisx SpatialRateNRSource in the tutorial spatial_hist is a class attribute and not an instance attribute.

This is an issue for if we want multiple spatially varying sources with different distributions later on.
@robertsjames suggested you take a look @JelleAalbers

Cheers,
Josh

@robertsjames
Copy link
Collaborator

Actually, I've thought about this a bit more, and I think it might be okay...

Because we direct the block attributes to the source attributes (see here), provided each class correctly sets its own spatial_hist, we should be good...

@josh0-jrg, I'd suggest testing this: create a detector-specific (i.e. LZ) SpatialRateNRSource, which can contain the map-checking feature we discussed. The create 2 specific spatially-dependent NR background sources inheriting from this, with different maps. Then, check those maps don't override each other!

@robertsjames
Copy link
Collaborator

I think you will have to make sure that the super().__getattribute__ method resolution order does direct to the spatial_hist in the 'bottom' derived class (in my example above, the guys inheriting from the detector-specific SpatialRateNRSource). @JelleAalbers , would be useful to hear what you think

@plt109
Copy link
Member

plt109 commented Nov 2, 2022

@Ashley-Joy, did you bump into the same issue as well? You mentioned having some troubles fitting NR with multiple sources to take care of the multiple-scatter contamination, but is it related to the issue reported here?

@robertsjames
Copy link
Collaborator

@plt109 did you just publicly admit to your MS woes? ;)

@josh0-jrg
Copy link
Author

josh0-jrg commented Nov 2, 2022

Works fine, have to assign call self.spatial_hist before the super().__init__(*args, **kwargs) when creating a source in background_sources.py - This is unlike the rate_vs_energy and energies attributes, why?

@JelleAalbers
Copy link
Member

Sorry, I missed this issue earlier. I can definitely imagine this is confusing, the documentation is not so great on this. If it helps:

spatial_hist from SpatialRateEnergySpectrum is a model attribute. You can set this (and other) attributes either as a class attribute (when you make a custom source class) or as an instance attribute through any of the methods described in #140.

However, background_sources.py (assuming we're looking at the same one from Rob's unmerged branches) sets model attributes in the custom __init__ of a source. (Maybe because the NEST sources predate #140 ?).

I suppose that works too, if indeed it's done before the super().__init__ call. If you do it afterwards, the rest of the source setup has already happened, which e.g. calculates a converted/normalized histogram from spatial_hist. The setup does nothing for attributes like energies of FixedShapeEnergySpectrum, so that one you can even set after the init without problems.

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

No branches or pull requests

4 participants