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

Populate Geometry/HGCalMapping with first version of the mapping files #1

Merged
merged 11 commits into from
May 3, 2024

Conversation

pfs
Copy link
Contributor

@pfs pfs commented Feb 7, 2024

This PR populates the newly created repository with the first version of the files.
The README.md is also updated with a minimal description and proposal of the versioning for now.

@kerstinlovisa

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2024

A new Pull Request was created by @pfs (Pedro Silva) for branch main.

@makortel, @bsunanda, @civanch, @smuzaffar, @srimanob, @aandvalenzuela, @subirsarkar, @Dr15Jones, @mdhildreth, @cmsbuild, @iarspider can you please review it and eventually sign? Thanks.
@rappoccio, @antoniovilela, @sextonkennedy you are the release manager for this.
cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2024

cms-bot internal usage

@srimanob
Copy link

srimanob commented Feb 8, 2024

+Upgrade

@pfs
Copy link
Contributor Author

pfs commented Feb 8, 2024

Thanks - it looks like the bot is waiting for someone with enough karma to issue a test command? If this can be integrated we can then proceed to update cms-sw/cmssw#43751

@smuzaffar
Copy link

@pfs , first there is no need to create data sub directory. If you access these files as Geometry/HGCalMapping/data/modulelocator_test.txt (https://github.com/cms-sw/cmssw/pull/43751/files#diff-1c02ef59051ccce7b4d7b0b9c03cde85a1ff304e9a9abe928a9d3e41d76e5b95R6) then you need to just add modulelocator_test.txt in this repository ( instead of data/ModuleMaps/v0/modulelocator_test.txt). If you want to keep these files under a sub directory e.g. ModuleMaps then please update cms-sw/cmssw#43751 PR to use Geometry/HGCalMapping/data/ModuleMaps/modulelocator_test.txt.

Are there any plans to have multiple version of these files with in same cmssw release? If yes then create a version sub-directory is fine but if only one version of this file is needed with in the CMSSW release then drop the v0 sub-directory.

I would suggest to already update cms-sw/cmssw#43751 ( remove the data files from there ) and start the testing from the cmssw PR.

@cmsbuild
Copy link
Contributor

Pull request #1 was updated.

@pfs
Copy link
Contributor Author

pfs commented Feb 19, 2024

hello @smuzaffar thanks for the suggestions. I prefer to keep the two separate folders {Cell,Module}Maps, but i removed the versioning and moved them to main directory.
The only different versions will happen for module maps, once we have the final optical fiber map and for test beam, but in the end it's preferrable to have these files with different names.
I'll update cms-sw/cmssw#43751. How can i test with the changes of this PR?
thanks!

@aandvalenzuela
Copy link

Hello @pfs, you can test directly from cms-sw/cmssw#43751 by typing please test with <url to data PR> as a comment when you are ready for testing

@pfs
Copy link
Contributor Author

pfs commented Feb 19, 2024

Ok i trigger the tests - hopefully it will work first time

@perrotta
Copy link

@pfs are these data files meant to be used only for tests *(now only accessed by Geometry/HGCalMapping/test/testMappingModuleIndexer_cfg.py in #43751)? If not, are they supposed to move eventually in the DB?

@pfs
Copy link
Contributor Author

pfs commented Feb 22, 2024

They will be eventually moved to conddb.

@pfs
Copy link
Contributor Author

pfs commented Mar 7, 2024

Hello - if no objections can this be merged?

@perrotta
Copy link

perrotta commented Mar 7, 2024

Hello - if no objections can this be merged?

In principle yes, because they are all new files which won't affect current workflows. But why should we hurry up, and not merge it together with the accompanying CMSSW PR #43751, once ready?

@pfs
Copy link
Contributor Author

pfs commented Mar 7, 2024

ah ok, yes sounds good. We'll try to converge on PR #43751 sometime next week after the hackathon - thanks

@cmsbuild
Copy link
Contributor

Pull request #1 was updated.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2024

REMINDER @rappoccio, @antoniovilela, @sextonkennedy: This PR was tested with cms-sw/cmssw#43751, please check if they should be merged together

@antoniovilela
Copy link

+1

@antoniovilela
Copy link

merge

@cmsbuild cmsbuild merged commit 6f4e1f2 into cms-data:main May 3, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants