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

update the ndlar yaml configuration #158

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Conversation

YifanC
Copy link
Collaborator

@YifanC YifanC commented Jan 28, 2025

tested with a mpvmpr sample.

Notifying @alexbooth92 @mjkramer

Copy link
Member

@alexbooth92 alexbooth92 left a comment

Choose a reason for hiding this comment

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

Thanks for the ping Yifan. I have some questions about your last commit. I think it changes the philosophy of the original implementation of the NDLAr workflow which was only add a yamls/ndlar_flow version of a particular yaml if the yaml/proto_nd_flow had an explicit dependency on NDLAr. This is to help with the NDLAr workflow automatically picking up any changes to the yamls/proto_nd_flow yamls. Do we need, for example, a

yamls/ndlar_flow/reco/charge/EventBuilder.yaml

or will

yamls/proto_nd_flow/reco/charge/EventBuilder.yaml

suffice?

@alexbooth92
Copy link
Member

Unrelated to this PR specifically but something that I just spotted - what is the reason we now have a

https://github.com/DUNE/ndlar_flow/blob/develop/data/ndlar_flow/ndlar-module.yaml

in develop when the install of ndlar_flow does a copy of this file from larnd-sim? In my opinion, relying on the install is the better way to do things. Whenever we have to change ndlar-module.yaml we have to do so in like 7 places already 😅 it is maximally confusing.

@YifanC
Copy link
Collaborator Author

YifanC commented Jan 28, 2025

Hi @alexbooth92, I thought the philosophy of using the folders of yaml configuration for different detectors is that each contains a complete set to reduce interdependence. We don't have a main set of configuration. In this way, each set of configuration has equal foot. On the other hand, the source code should be shared among different run configuration.

@alexbooth92
Copy link
Member

Hi @alexbooth92, I thought the philosophy of using the folders of yaml configuration for different detectors is that each contains a complete set to reduce interdependence. We don't have a main set of configuration. In this way, each set of configuration has equal foot. On the other hand, the source code should be shared among different run configuration.

Thanks Yifan! I don't mind moving to this model if 2x2 developers remember to change the corresponding NDLAr yaml whenever they adapt a 2x2 yaml for a reason that is applicable to both.

@YifanC
Copy link
Collaborator Author

YifanC commented Feb 4, 2025

Unrelated to this PR specifically but something that I just spotted - what is the reason we now have a

https://github.com/DUNE/ndlar_flow/blob/develop/data/ndlar_flow/ndlar-module.yaml

in develop when the install of ndlar_flow does a copy of this file from larnd-sim? In my opinion, relying on the install is the better way to do things. Whenever we have to change ndlar-module.yaml we have to do so in like 7 places already 😅 it is maximally confusing.

Hi @alexbooth92 ! Sorry I forgot to reply this. If you run scripts/ndlar_scripts/get_ndlar_input.sh again (or the usual procedure), it will overwrite the old files as long as the file names are the same. In that sense, adding a default data folder as for the single modules and 2x2 should not change the workflow and would not require manual updates of the configuration. I think it helps for quick tests or so.

It seems to me that you are happy with the PR change itself from the above comments. Please let me know if you have further comments or confirm it is alright.

@alexbooth92
Copy link
Member

Unrelated to this PR specifically but something that I just spotted - what is the reason we now have a
https://github.com/DUNE/ndlar_flow/blob/develop/data/ndlar_flow/ndlar-module.yaml
in develop when the install of ndlar_flow does a copy of this file from larnd-sim? In my opinion, relying on the install is the better way to do things. Whenever we have to change ndlar-module.yaml we have to do so in like 7 places already 😅 it is maximally confusing.

Hi @alexbooth92 ! Sorry I forgot to reply this. If you run scripts/ndlar_scripts/get_ndlar_input.sh again (or the usual procedure), it will overwrite the old files as long as the file names are the same. In that sense, adding a default data folder as for the single modules and 2x2 should not change the workflow and would not require manual updates of the configuration. I think it helps for quick tests or so.

It seems to me that you are happy with the PR change itself from the above comments. Please let me know if you have further comments or confirm it is alright.

In the interest of moving forward, happy for this to be merged but making a plea for 2x2 folk to update the NDLAr yamls whenever relevent! There seems to have been a real effort to do so recently so that's great!

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