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 optics #13

Merged
merged 32 commits into from
Mar 29, 2022
Merged

Generic optics #13

merged 32 commits into from
Mar 29, 2022

Conversation

jovoy
Copy link
Contributor

@jovoy jovoy commented Feb 21, 2022

Large jovoy 720

Changes proposed in this pull-request:

  • TRestAxionOptics implements now virtual methods (GetPositionAtExit, GetDirectionAtExit and GetEfficiency that need to be implemented at the inherited class, i.e. TRestAxionGenericOptics or TRestAxionMCPLOptics.
  • TRestAxionGenericOptics implementation of optics particle tracking using full mirror shell geometry description and material reflectivity. Skeleton class has been added. But needs to integrated. (WIP).

Reminder: New features and upgrades should be sufficiently documented and a corresponding validation test at the pipeline should be implemented.

@rest-for-physics/axionlib

Double_t fSpiderWidth = TMath::Pi() / 18. / 4.; //<

/// The spider structure will be effective from this radius, in mm. Default is from 0 mm.
Double_t fSpiderStartRadius = 0.; //<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should allow for a hole here as there are no mirrors in the middle anyways and we are planning it for the optics alignment. So maybe the default should be greater than 0mm (maybe like 20mm as the innermost radius of the new BabyIAXO optics probably can't be smaller than 54mm).

Copy link
Member

Choose a reason for hiding this comment

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

Sure, thats the idea for that data member. Yes, probably is a good idea to include a non-zero default value.

I introduced it here:
906028d

Change it at will.

Double_t r2 = r1 - fLength * sin(angle);
// r1 = sqrt((pos[0] + s * dir[0]) * (pos[0] + s * dir[0]) +
// (pos[1] + s * dir[1]) * (pos[1] + s * dir[1])) -
// ((r2 - r1) * (pos[2] + s * dir[2] - distMirrors) / (cos(angle) * fLength)) //needs to be solved for s, maybe by incresing s for some value and comparing this to r1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe someone knows a better way in c++ to get s from this equation than to increase s slowly and comparing r1 to the rest of the term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All values except for s are known

Copy link
Member

@jgalan jgalan Feb 23, 2022

Choose a reason for hiding this comment

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

With ROOT you may define a TF1 object, which is a maths function, then use the Solver to get the roots of that function. I found this tutorial with a quick search:

https://www.youtube.com/watch?v=7VgmFpxzK9M

Copy link
Member

Choose a reason for hiding this comment

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

You will likely need to add the #include <TF1.h> header on your cxx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that sounds great! For the interval I would maybe need to know how long the direction vectors are approximately and where the position vectors are approximately. If the position in in the sun and the direction vector goes from sun to magnet the interval should be around 1.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't look to the details, but I would expect the input position is where the particle hits the entrace plane, i.e. the position returned by TRestAxionOptics::GetPositionAtEntrance, while the output position is the position at the exit optics plane, i.e. that needs to be calculated by each specific optics process, the position should belong to the exit optics plane. And it must be implemented in a function named TRestAxionGenericOptics::GetPositionAtExit.

The input direction vector will be normalized to unity, and therefore it should be the output direction returned by TRestAxionGenericOptics::GetDirectionAtExit

Copy link
Member

Choose a reason for hiding this comment

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

So, I don't know what you mean by the interval, but that interval should probably be placed inside the optics volume or domain.

The propagation from the Sun (or the magnet exit) to the optics plane entrance is basically done by TRestAxionOptics::GetPositionAtEntrance that uses REST_Physics::MoveToPlane to find the intersection of a straight line with the optics plane entrance.

@jgalan
Copy link
Member

jgalan commented Feb 23, 2022

@jovoy as you can see, the pipeline is not passing because there is a compilation error.

Screenshot 2022-02-23 at 16 50 41

As it can be seen at the header of the PR description:

Large jovoy 469

If you click at the badge icon then you get to the pipeline output, where it seems the problem is coming because the inherited class TRestAxionGenericOptics is trying to access private members of TRestAxionOptics. This is probably solved by doing the members you need protected, instead of private. We will never make them public.

@jgalan
Copy link
Member

jgalan commented Feb 23, 2022

Probably you also need to test the build locally ;p

/// predetermined layer
///
TVector3 TRestAxionGenericOptics::GetInteractionPoint(const TVector3& pos, const TVector3& dir) {
Int_t layer = TRestAxionOptics::GetEntranceShell(pos, dir);
Copy link
Member

@jgalan jgalan Feb 23, 2022

Choose a reason for hiding this comment

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

Line 141 looks strange. There is no need to call specifying the base class, mainly because the method GetEntranceShell is not virtual, and it is not re-implemented inside TRestAxionGenericOptics. Therefore, just calling GetEntranceShell will have the same effect. When the method might be existing on both, base and inherited, then we need to specify, and therefore, as it is written now, to me it looks as if GetEntranceShell is on both classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay, I understand, thanks!

@jgalan
Copy link
Member

jgalan commented Feb 24, 2022

I have created a validation script that generates the figure shown at the documentation. The documentation is updated automatically every night at https://sultan.unizar.es/rest/ once the changes have been merged to master.

In the meantime I have manually executed doxygen REST.conf at the doc/doxygen/ directory, and placed it at a temporary place:

https://sultan.unizar.es/test/classTRestAxionOptics.html#a1317a5414db13e46bbabf5f54ba54780

This is generated by pipeline/optics/basic.py
opticsBasic-4

@jgalan jgalan marked this pull request as ready for review March 29, 2022 14:58
@jgalan
Copy link
Member

jgalan commented Mar 29, 2022

As we discussed online we merge this PR so that we focus on the coming updates on these classes. TRestAxionOptics and TRestAxionGenericOptics.

@jgalan jgalan merged commit d9b0bd3 into master Mar 29, 2022
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.

2 participants