rat-pac / ratpac-two

Open source edition of RAT-PAC
Other
11 stars 25 forks source link

Update dichroic filter implementation #79

Closed JamesJieranShen closed 1 year ago

JamesJieranShen commented 1 year ago

Improves upon the the work done in #64.

tannerbk commented 1 year ago

Looks good -- I'm really not certain what the right thing to do about the Geant4 prints other than this type of hack.

Also, I think the specific dichroic data should probably be added to EosSimulations rather than to ratpac-two? I'm not sure, but if the dichroic data are bench-top measurements at Penn, perhaps they should not be made publicly available (if there is a chance they're part of a future paper?) @smnaugle may have thoughts. (Note: this repo will soon be made public).

smnaugle commented 1 year ago

@tannerbk That is a good point I hadn't thought about. Yes, Amanda is currently writing a paper presenting these measurements, so maybe it is best to put those in EosSimulations for now.

JamesJieranShen commented 1 year ago

Thanks for pointing that out, @tannerbk and @smnaugle! I have just removed both files from the repo for now. To ensure these files are not present in the commit history, this PR should be merged with a squashed commit, rather than a regular merge commit.

As far as I understand it, if this repo becomes public at some point, my fork will be de-forked on github and remain private, so no trace of the data files should be found through this PR or my private commits.

tannerbk commented 1 year ago

Sorry, I still see the .dat data files in this PR. Is that public data that you want to include?

JamesJieranShen commented 1 year ago

The remaining data files were measurements made by Meng in 2020, which I assumed were already published? @smnaugle could you confirm whether that can be made public?

smnaugle commented 1 year ago

I think these measurements were a starting point for Amanda, and also made with the spectrophotometer. If we aren't including Amanda's data I don't think we should include these either. Once the paper is published we can put them in the public repo though.

JamesJieranShen commented 1 year ago

OK, thanks @smnaugle for clarifying. I've amended the previous commit and removed all existing data files. @tannerbk is there anything else I should incorporate here?

tannerbk commented 1 year ago

I think you should commit the data to the EosSimulations repository and we can merge them together. And in this PR, it would be useful to have placeholder data, maybe from the manufacturer.

smnaugle commented 1 year ago

I think manufacturer values as a placeholder is a good idea. @JamesJieranShen this link shows the behavior for a 450nm shortpass filter from Knight, and you can find the behavior of other filters by looking around their website. You could use a data grabber on the pdfs.

JamesJieranShen commented 1 year ago

Thanks for sending the link @smnaugle! I've linearly interpolated the data for 450nm shortpass and 480 longpass, and added them to the repo (The linear interpolation is necessary as G4's default bicubic interpolation will shoot the transmission above 100). I'm assuming that I should be propagating this entire PR along with the measured values to the EosSimulation repository. Should I create a mirroring PR on that repo, or do changes typically propagate downstream through some other means?