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

Generic wls fiber #183

Merged
merged 8 commits into from
Oct 23, 2023
Merged

Generic wls fiber #183

merged 8 commits into from
Oct 23, 2023

Conversation

FabianKellerer
Copy link

Added the GenericWLSFiber geometry. Updated the MaterialsList to include the EJ-286 fiber core material.

Copy link
Contributor

@jmalbos jmalbos left a comment

Choose a reason for hiding this comment

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

The geometry assumes now that the external coating will always be TPB, and that won't always be the case. Also, I don't see any optical material properties attached to the inner volumes of the fiber. Those should be incorporated into this class, giving the user the option to pass them via a function, as we do in the generic photosensor.

source/geometries/GenericWLSFiber.cc Outdated Show resolved Hide resolved
source/geometries/GenericWLSFiber.h Outdated Show resolved Hide resolved
source/geometries/GenericWLSFiber.h Outdated Show resolved Hide resolved
source/geometries/GenericWLSFiber.h Outdated Show resolved Hide resolved
source/materials/MaterialsList.cc Outdated Show resolved Hide resolved
source/geometries/GenericWLSFiber.h Outdated Show resolved Hide resolved
@soleti
Copy link
Contributor

soleti commented Feb 6, 2023

Hello, I was wondering if there were any updates about this. If the code is fine it would be helpful for us working on fibers simulations to merge it.

@paolafer
Copy link
Contributor

paolafer commented Feb 7, 2023

@jmalbos is the reviewer, how is it going, Justo?

@jmalbos
Copy link
Contributor

jmalbos commented Feb 7, 2023

@jmalbos is the reviewer, how is it going, Justo?

I'll try to finish the review asap, but, most likely, I won't be able till next week.

@paolafer
Copy link
Contributor

paolafer commented Jul 5, 2023

Hi, everybody! How are we going with this PR? As far as I know, several people are using this geometry, without the code being approved, and I think it's not optimal. Could we move this forward? @jmalbos, @soleti?

@paolafer paolafer requested a review from soleti July 20, 2023 08:22
Copy link
Contributor

@soleti soleti left a comment

Choose a reason for hiding this comment

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

Apart few small things, I don't see any obvious problem in the code (which I have been using for a while after all...). However, before merging, I think it would be nice to verify that we reproduce some fundamental properties of the WLS fibers. At least two plots might be added for completeness to this PR (maybe they already exist but I couldn't find them on DocDB):

  • attenuation length: light collected as a function of the distance from a generic photosensor
  • trapping efficiency: amount of light collected by generating photons at the center of the fiber.

source/geometries/GenericWLSFiber.cc Outdated Show resolved Hide resolved
source/geometries/GenericWLSFiber.cc Outdated Show resolved Hide resolved
fiber_mat_->SetMaterialPropertiesTable(opticalprops::EJ280());
}
else if (fiber_mat_name_ == "EJ286") {
fiber_mat_ = materials::EJ280(); // Same base material than EJ280
fiber_mat_ = materials::PVT(); // Same base material than EJ280
Copy link
Contributor

Choose a reason for hiding this comment

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

By looking at the code PVT is not exactly the same as the EJ280 material you are deleting. Can you explain why?

Copy link
Contributor

@paolafer paolafer Oct 17, 2023

Choose a reason for hiding this comment

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

In a previous comment, @jmalbos said that EJ280 is the same as PVT, except for the doping. However, this is not what our code says (the composition in terms of H and C is different). We need to be sure about what was wrong in the code, before eliminating EJ280 instead of PVT, for instance.

@paolafer
Copy link
Contributor

@FabianKellerer, how is this PR going?

@FabianKellerer
Copy link
Author

Apart few small things, I don't see any obvious problem in the code (which I have been using for a while after all...). However, before merging, I think it would be nice to verify that we reproduce some fundamental properties of the WLS fibers. At least two plots might be added for completeness to this PR (maybe they already exist but I couldn't find them on DocDB):

* attenuation length: light collected as a function of the distance from a generic photosensor

* trapping efficiency: amount of light collected by generating photons at the center of the fiber.

This information can be found in the attached file. The trapping efficiency given by the manufacturer is reproduced very well, and the attenuation is aprrox. 60% per meter.
Fibre Trapping Efficiencies.pdf

@soleti
Copy link
Contributor

soleti commented Sep 27, 2023

Thank you @FabianKellerer for the slides, very informative. The only remaining doubt I have is about the attenuation length. For e.g. Kuraray Y11 fibers that have an attenuation length of >3.5 m we should lose only ~25% of the light and not 60% after 1 m. Or am I misinterpreting your numbers? Thanks!

@paolafer
Copy link
Contributor

@FabianKellerer, could you have a look at this last question from Roberto, so that we can approve the PR? Thanks!

@FabianKellerer
Copy link
Author

Thank you @FabianKellerer for the slides, very informative. The only remaining doubt I have is about the attenuation length. For e.g. Kuraray Y11 fibers that have an attenuation length of >3.5 m we should lose only ~25% of the light and not 60% after 1 m. Or am I misinterpreting your numbers? Thanks!

I made a plot comparing light loss in the simulation to the light loss measured by Kuraray (Link) The agreement seems quite good.
AttenuationLength

@soleti
Copy link
Contributor

soleti commented Oct 23, 2023

This looks good to me! I approve the PR.

@paolafer
Copy link
Contributor

The code is not compiling!

@paolafer
Copy link
Contributor

My compiler is also issuing the following compilation warning:
source/geometries/GenericWLSFiber.cc:48:3: warning: field 'coating_optProp_' will be initialized after field 'core_mat_' [-Wreorder-ctor] coating_optProp_(nullptr).

To avoid this kind of warnings, it is a good rule to declare variables in the header file and to initialize them in the .cc file in the same order. Can you fix that, too?

@paolafer paolafer merged commit 0a9e51c into next-exp:master Oct 23, 2023
1 check passed
@FabianKellerer FabianKellerer deleted the GenericWLSFiber branch April 17, 2024 09:13
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