portelli/HadronsPresets
1
0
Fork 1
Code Issues Pull Requests Packages Projects Releases Wiki Activity

adding C1M and restructuring solvers #1

Merged
portelli merged 5 commits from mdicarlo/HadronsPresets:main into main 2023-06-01 16:39:14 +01:00
Conversation 6 Commits 5 Files Changed 1
+249 -19
mdicarlo commented 2023-05-22 17:12:54 +01:00
Contributor
Copy Link
Copy Source

Added inline to static constexpr EnsembleParameters because otherwise I got a linker error.

Added `inline` to `static constexpr EnsembleParameters` because otherwise I got a linker error.
mdicarlo added 1 commit 2023-05-22 17:12:55 +01:00
adding C1M and restructuring solvers 3595f7d250
portelli commented 2023-05-25 14:17:58 +01:00
Owner
Copy Link
Copy Source

@mdicarlo thanks for the C1M PR to HadronsPreset, although I think there is a number of issues with it which require some refactoring:

  • the term "LCD" stands for local coherence deflation, and it should not be used to name solver not using this setup (so far it is only used with M0).
  • I would not go with the addLightLCDSolver generalisation proposed here, the compression parameters in it are hardcoded to the M0 one, and how this would generalise is non-trivial. So I would propose we just keep the addM0LightLCDSolver as it was before until a use case for generalisation shows up.
  • I can see periodic boundary conditions in time are hardcoded in several cases, I did that for M0 as the EV we have were computed with periodic BC. But we generally use anti-periodic in time so I am not sure if you want to carry that over to C1M.
@mdicarlo thanks for the C1M PR to HadronsPreset, although I think there is a number of issues with it which require some refactoring: - the term "LCD" stands for local coherence deflation, and it should not be used to name solver not using this setup (so far it is only used with M0). - I would not go with the `addLightLCDSolver` generalisation proposed here, the compression parameters in it are hardcoded to the M0 one, and how this would generalise is non-trivial. So I would propose we just keep the `addM0LightLCDSolver` as it was before until a use case for generalisation shows up. - I can see periodic boundary conditions in time are hardcoded in several cases, I did that for M0 as the EV we have were computed with periodic BC. But we generally use anti-periodic in time so I am not sure if you want to carry that over to C1M.
ferben commented 2023-05-25 14:37:30 +01:00
First-time contributor
Copy Link
Copy Source

Thanks Antonin, that all makes sense. I guess we can address the points by

  • calling things "c1mIRLPar", "addC1mRunimeIRLSolver", etc.
  • revert to "addM0LightLCDSolver"
  • change boundary conditions to antiperiodic - thanks for spotting that!
Thanks Antonin, that all makes sense. I guess we can address the points by - calling things "c1mIRLPar", "addC1mRunimeIRLSolver", etc. - revert to "addM0LightLCDSolver" - change boundary conditions to antiperiodic - thanks for spotting that!
portelli commented 2023-05-25 14:57:36 +01:00
Owner
Copy Link
Copy Source

@ferben thanks for the quick response:

calling things "c1mIRLPar", "addC1mRunimeIRLSolver", etc.

yes that works for me

revert to "addM0LightLCDSolver"

thanks

change boundary conditions to antiperiodic - thanks for spotting that!

something more needs to be done about that, if you want to use higher level functions to avoid duplication then I would think this needs to be a parameter of it. This can just be a boolean as we very unlikley will do anything else than perdiodic/antiperiodic. However I would still hardcode it for a given setup

@ferben thanks for the quick response: > calling things "c1mIRLPar", "addC1mRunimeIRLSolver", etc. yes that works for me > revert to "addM0LightLCDSolver" thanks > change boundary conditions to antiperiodic - thanks for spotting that! something more needs to be done about that, if you want to use higher level functions to avoid duplication then I would think this needs to be a parameter of it. This can just be a boolean as we very unlikley will do anything else than perdiodic/antiperiodic. However I would still hardcode it for a given setup
mdicarlo added 3 commits 2023-05-26 16:09:44 +01:00
reverting to addM0LightLCDSolver ab747d2b1d
changing BC to antiperiodic 283136764c
changing LCD into IRL for C1M b4d4dcccd0
mdicarlo commented 2023-05-26 16:11:35 +01:00
Author
Contributor
Copy Link
Copy Source

Thanks @portelli, I agree with all the three points above.
I fixed the code accordingly, following @ferben suggestion.

Thanks @portelli, I agree with all the three points above. I fixed the code accordingly, following @ferben suggestion.
portelli commented 2023-05-26 17:13:36 +01:00
Owner
Copy Link
Copy Source

Thank you both, however it seems that the BC issue still needs to be addressed?

Thank you both, however it seems that the BC issue still needs to be addressed?
mdicarlo commented 2023-05-26 17:18:58 +01:00
Author
Contributor
Copy Link
Copy Source

You are right, I fixed that for the light solver but not for the strange.

What do you think about redefining the functions with an additional argument like

  static inline void addM0LightLCDSolver(Application &app, const std::string solverName,
                                         const std::string gaugeName,
                                         const std::string gaugeTransform,
                                         const std::string eigenpackPath,
                                         const std::string boundary = "1 1 1 1",
                                         const double residual = 1.0e-8);

  static inline void addC1MLightRuntimeIRLSolver(Application &app, const std::string solverName,
                                                 const std::string gaugeName,
                                                 const std::string gaugeTransform,
                                                 const std::string boundary = "1 1 1 -1",
                                                 const double residual = 1.0e-8);

Or the string boundary could be added to the struct EnsembleParameters.

You are right, I fixed that for the light solver but not for the strange. What do you think about redefining the functions with an additional argument like ``` static inline void addM0LightLCDSolver(Application &app, const std::string solverName, const std::string gaugeName, const std::string gaugeTransform, const std::string eigenpackPath, const std::string boundary = "1 1 1 1", const double residual = 1.0e-8); ``` ``` static inline void addC1MLightRuntimeIRLSolver(Application &app, const std::string solverName, const std::string gaugeName, const std::string gaugeTransform, const std::string boundary = "1 1 1 -1", const double residual = 1.0e-8); ``` Or the string `boundary` could be added to the struct `EnsembleParameters`.
mdicarlo added 1 commit 2023-05-27 14:54:59 +01:00
boundary as argument of solver functions 9db87de9f9
portelli merged commit b817338254 into main 2023-06-01 16:39:14 +01:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
portelli (Antonin Portelli)
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: portelli/HadronsPresets#1