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

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
portelli commented 2023-05-25 14:17:58 +01:00
Owner
Copy Link

@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

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

@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
mdicarlo commented 2023-05-26 16:11:35 +01:00
Author
Contributor
Copy Link

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

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

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
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 Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
portelli
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: portelli/HadronsPresets#1
No description provided.
Delete Branch "mdicarlo/HadronsPresets:main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?