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

Iss1117 #1135

Merged
merged 47 commits into from
May 10, 2023
Merged

Iss1117 #1135

merged 47 commits into from
May 10, 2023

Conversation

EinarElen
Copy link
Contributor

@EinarElen EinarElen commented Dec 5, 2022

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

This resolves #1117. This primarily means providing the length of a scintillator bar given an HcalID from the geometry condition. Since the side hcal for v14 has different length of bars depending on the layer number, this needs to be a function of both section and layer.

Example use would be

auto barLength {hcalGeometry.getScintillatorLength(id); 

Check List

  • I successfully compiled ldmx-sw with my developments
  • I ran my developments and the following shows that they are successful.
    To check that these changes didn't change the output of the condition, I added the following to the end of configure step
  auto section_to_string{[](const ldmx::HcalID id) { // Really wish we would have reflection in C++
    switch (id.section()) {
      case ldmx::HcalID::HcalSection::BACK:
        return "BACK";
      case ldmx::HcalID::HcalSection::TOP:
        return "TOP";
      case ldmx::HcalID::HcalSection::BOTTOM:
        return "BOTTOM";
      case ldmx::HcalID::HcalSection::LEFT:
        return "LEFT";
      case ldmx::HcalID::HcalSection::RIGHT:
        return "RIGHT";
    }
  }};
  std::cout << std::boolalpha;
  std::cout << "HcalGeo->Geometry has " << getNumSections()
            << " sections, a scintillator width of " << getScintillatorWidth()
            << "mm, Ecal dx/dy of " << getEcalDx() << "/" << getEcalDy()
            << " mm" << std::endl;
  for (auto section{0}; section < getNumSections(); ++section) {
    std::cout << "\tHcalGeo->Section: " << section_to_string({section, 1, 2})
              << " has " << num_layers_[section] << " layers\n";
    for (auto layer{1}; layer <= getNumLayers(section); ++layer) {
      // Number of strips per layer
      std::cout << "\tHcalGeo-> S: " << section << ", L: " << layer << " has "
                << getNumStrips(section, layer)
                << " strips, Zero Strip: " << getZeroStrip(section, layer)
                << ", halfTotalWidth: " << getHalfTotalWidth(section, layer)
                << " mm";
      if (section == ldmx::HcalID::HcalSection::BACK) {
        std::cout << " is horizontal? " << layerIsHorizontal(layer);
      }
      std::cout << std::endl;
      for (auto strip{0}; strip < getNumStrips(section, layer); ++strip) {
        ldmx::HcalID id{section, layer, strip};
        auto pos{getStripCenterPosition(id)};
        std::cout << "\t\tHcalGeo->" << id << " center position: " << pos[0]
                  << '/' << pos[1] << '/' << pos[2] << std::endl;
      }
    }
  }

I then ran this for the v13, v14, and prototype v2 geometries along the lines of

ldmx fire config.py | grep HcalGeo >> v13_iss1117.txt 

for both the iss1117 branch and trunk and then checked

diff v13_iss1117.txt v13_trunk.txt # etc
  • I attached any sub-module related changes to this PR.

Related Sub-Module PRs

@EinarElen
Copy link
Contributor Author

Here's an incredibly 24.00 in the evening test that we are getting it right, adding:

    for (auto section{0}; section < num_sections_; ++section) {
      std::cout << "Section: " << section_to_string({section, 1, 2})
                << std::endl;
      for (auto layer{1}; layer <= num_layers_[section]; ++layer) {
        if (section == 0) {
          std::cout << "First layer has length: "
                    << getScintillatorLength({section, layer, 0}) << "mm\n";
          std::cout << "Alternate layer has length: "
                    << getScintillatorLength({section, layer + 1, 0}) << "mm\n";
          break;
        } else {
          int strip{0};
          const ldmx::HcalID id{section, layer, strip};
          std::cout << "Layer: " << layer << std::setw(2) << " has length "
                    << getScintillatorLength(id) << "mm\n";
        }
      }
    }

at the bottom of the configure function gives:

V13 geometry
Section: BACK
First layer has length: 3100mm
Alternate layer has length: 3100mm
Section: TOP
Layer: 1 has length 2500mm
Layer: 2 has length 2500mm
Layer: 3 has length 2500mm
Layer: 4 has length 2500mm
Layer: 5 has length 2500mm
Layer: 6 has length 2500mm
Layer: 7 has length 2500mm
Layer: 8 has length 2500mm
Layer: 9 has length 2500mm
Layer: 10 has length 2500mm
Layer: 11 has length 2500mm
Layer: 12 has length 2500mm
Layer: 13 has length 2500mm
Layer: 14 has length 2500mm
Layer: 15 has length 2500mm
Layer: 16 has length 2500mm
Layer: 17 has length 2500mm
Layer: 18 has length 2500mm
Layer: 19 has length 2500mm
Layer: 20 has length 2500mm
Layer: 21 has length 2500mm
Layer: 22 has length 2500mm
Layer: 23 has length 2500mm
Layer: 24 has length 2500mm
Layer: 25 has length 2500mm
Layer: 26 has length 2500mm
Layer: 27 has length 2500mm
Layer: 28 has length 2500mm
Section: BOTTOM
Layer: 1 has length 2500mm
Layer: 2 has length 2500mm
Layer: 3 has length 2500mm
Layer: 4 has length 2500mm
Layer: 5 has length 2500mm
Layer: 6 has length 2500mm
Layer: 7 has length 2500mm
Layer: 8 has length 2500mm
Layer: 9 has length 2500mm
Layer: 10 has length 2500mm
Layer: 11 has length 2500mm
Layer: 12 has length 2500mm
Layer: 13 has length 2500mm
Layer: 14 has length 2500mm
Layer: 15 has length 2500mm
Layer: 16 has length 2500mm
Layer: 17 has length 2500mm
Layer: 18 has length 2500mm
Layer: 19 has length 2500mm
Layer: 20 has length 2500mm
Layer: 21 has length 2500mm
Layer: 22 has length 2500mm
Layer: 23 has length 2500mm
Layer: 24 has length 2500mm
Layer: 25 has length 2500mm
Layer: 26 has length 2500mm
Layer: 27 has length 2500mm
Layer: 28 has length 2500mm
Section: RIGHT
Layer: 1 has length 2300mm
Layer: 2 has length 2300mm
Layer: 3 has length 2300mm
Layer: 4 has length 2300mm
Layer: 5 has length 2300mm
Layer: 6 has length 2300mm
Layer: 7 has length 2300mm
Layer: 8 has length 2300mm
Layer: 9 has length 2300mm
Layer: 10 has length 2300mm
Layer: 11 has length 2300mm
Layer: 12 has length 2300mm
Layer: 13 has length 2300mm
Layer: 14 has length 2300mm
Layer: 15 has length 2300mm
Layer: 16 has length 2300mm
Layer: 17 has length 2300mm
Layer: 18 has length 2300mm
Layer: 19 has length 2300mm
Layer: 20 has length 2300mm
Layer: 21 has length 2300mm
Layer: 22 has length 2300mm
Layer: 23 has length 2300mm
Layer: 24 has length 2300mm
Layer: 25 has length 2300mm
Layer: 26 has length 2300mm
Section: LEFT
Layer: 1 has length 2300mm
Layer: 2 has length 2300mm
Layer: 3 has length 2300mm
Layer: 4 has length 2300mm
Layer: 5 has length 2300mm
Layer: 6 has length 2300mm
Layer: 7 has length 2300mm
Layer: 8 has length 2300mm
Layer: 9 has length 2300mm
Layer: 10 has length 2300mm
Layer: 11 has length 2300mm
Layer: 12 has length 2300mm
Layer: 13 has length 2300mm
Layer: 14 has length 2300mm
Layer: 15 has length 2300mm
Layer: 16 has length 2300mm
Layer: 17 has length 2300mm
Layer: 18 has length 2300mm
Layer: 19 has length 2300mm
Layer: 20 has length 2300mm
Layer: 21 has length 2300mm
Layer: 22 has length 2300mm
Layer: 23 has length 2300mm
Layer: 24 has length 2300mm
Layer: 25 has length 2300mm
Layer: 26 has length 2300mm
V14 geometry
Section: BACK
First layer has length: 2000mm
Alternate layer has length: 2000mm
Section: TOP
Layer: 1 has length 600mm
Layer: 2 has length 1800mm
Layer: 3 has length 600mm
Layer: 4 has length 1800mm
Layer: 5 has length 600mm
Layer: 6 has length 1800mm
Layer: 7 has length 600mm
Layer: 8 has length 1800mm
Layer: 9 has length 600mm
Layer: 10 has length 1600mm
Layer: 11 has length 600mm
Layer: 12 has length 1600mm
Layer: 13 has length 600mm
Layer: 14 has length 1600mm
Layer: 15 has length 600mm
Layer: 16 has length 1400mm
Layer: 17 has length 600mm
Layer: 18 has length 1400mm
Layer: 19 has length 600mm
Layer: 20 has length 1200mm
Layer: 21 has length 600mm
Layer: 22 has length 1200mm
Layer: 23 has length 600mm
Layer: 24 has length 1200mm
Section: BOTTOM
Layer: 1 has length 600mm
Layer: 2 has length 1800mm
Layer: 3 has length 600mm
Layer: 4 has length 1800mm
Layer: 5 has length 600mm
Layer: 6 has length 1800mm
Layer: 7 has length 600mm
Layer: 8 has length 1800mm
Layer: 9 has length 600mm
Layer: 10 has length 1600mm
Layer: 11 has length 600mm
Layer: 12 has length 1600mm
Layer: 13 has length 600mm
Layer: 14 has length 1600mm
Layer: 15 has length 600mm
Layer: 16 has length 1400mm
Layer: 17 has length 600mm
Layer: 18 has length 1400mm
Layer: 19 has length 600mm
Layer: 20 has length 1200mm
Layer: 21 has length 600mm
Layer: 22 has length 1200mm
Layer: 23 has length 600mm
Layer: 24 has length 1200mm
Section: RIGHT
Layer: 1 has length 600mm
Layer: 2 has length 1800mm
Layer: 3 has length 600mm
Layer: 4 has length 1800mm
Layer: 5 has length 600mm
Layer: 6 has length 1800mm
Layer: 7 has length 600mm
Layer: 8 has length 1800mm
Layer: 9 has length 600mm
Layer: 10 has length 1600mm
Layer: 11 has length 600mm
Layer: 12 has length 1600mm
Layer: 13 has length 600mm
Layer: 14 has length 1600mm
Layer: 15 has length 600mm
Layer: 16 has length 1400mm
Layer: 17 has length 600mm
Layer: 18 has length 1400mm
Layer: 19 has length 600mm
Layer: 20 has length 1200mm
Layer: 21 has length 600mm
Layer: 22 has length 1200mm
Layer: 23 has length 600mm
Layer: 24 has length 1200mm
Section: LEFT
Layer: 1 has length 600mm
Layer: 2 has length 1800mm
Layer: 3 has length 600mm
Layer: 4 has length 1800mm
Layer: 5 has length 600mm
Layer: 6 has length 1800mm
Layer: 7 has length 600mm
Layer: 8 has length 1800mm
Layer: 9 has length 600mm
Layer: 10 has length 1600mm
Layer: 11 has length 600mm
Layer: 12 has length 1600mm
Layer: 13 has length 600mm
Layer: 14 has length 1600mm
Layer: 15 has length 600mm
Layer: 16 has length 1400mm
Layer: 17 has length 600mm
Layer: 18 has length 1400mm
Layer: 19 has length 600mm
Layer: 20 has length 1200mm
Layer: 21 has length 600mm
Layer: 22 has length 1200mm
Layer: 23 has length 600mm
Layer: 24 has length 1200mm
Prototype geometry
Section: BACK
First layer has length: 2000mm
Alternate layer has length: 2000mm

@cmantill to me the v14 ordering looks good, but its the kind of thing where having a second pair of eyes on would be good. Does the order of z vs x/y layers and the number of x/y layers per module look good?

For the v13 geometry, I just dropped some dummy values for the side hcal length. Do you know what the actual lengths were @cmantill?

Copy link
Contributor

@cmantill cmantill left a comment

Choose a reason for hiding this comment

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

Looks good! Small comments for documentation and hopefully v13 is right (is complicated so feel free to correct me)

DetDescr/include/DetDescr/HcalGeometry.h Outdated Show resolved Hide resolved
DetDescr/include/DetDescr/HcalGeometry.h Show resolved Hide resolved
DetDescr/python/HcalGeometry.py Outdated Show resolved Hide resolved
DetDescr/python/HcalGeometry.py Outdated Show resolved Hide resolved
DetDescr/python/HcalGeometry.py Show resolved Hide resolved
DetDescr/src/DetDescr/HcalGeometry.cxx Show resolved Hide resolved
DetDescr/src/DetDescr/HcalGeometry.cxx Outdated Show resolved Hide resolved
@EinarElen
Copy link
Contributor Author

Ive edited the original post but since it is important i'm also noting here that the PR now includes two submodule PRs

Related Sub-Module PRs

@EinarElen EinarElen marked this pull request as draft April 27, 2023 18:13
@EinarElen EinarElen marked this pull request as ready for review April 27, 2023 18:14
@EinarElen
Copy link
Contributor Author

Note: I think merging this can wait until we have more progress on #1136 since that will be a good opportunity to test the features.

@EinarElen
Copy link
Contributor Author

Ok, after having relied on it a bunch in #1136, I think this is ready to go. I think we should have separate PRs for this and #1136 though, so we know that this PR didn't break any of the existing validation samples.

@cmantill
Copy link
Contributor

This sounds good to me. No reason to not integrate this.

@EinarElen
Copy link
Contributor Author

EinarElen commented May 10, 2023

Ok, let's just wait for a check from @tomeichlersmith. I don 't think this needs to have a dedicated release be created, since I'd like to get #1136 in as well

@EinarElen EinarElen mentioned this pull request May 10, 2023
3 tasks
Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

This looks good from my end :) thanks for reminding me @EinarElen

For merging, I would request that the submodule PRs are merged first, then this is updated to use those new commits on the submodule trunks. I'm away at conference/meeting this week - so feel free to poke me again next week if you need help going through that procedure.

@EinarElen
Copy link
Contributor Author

Should be up now!

@EinarElen EinarElen merged commit 1d6239b into trunk May 10, 2023
1 check passed
@EinarElen EinarElen deleted the iss1117 branch May 10, 2023 17:32
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.

Hcal geometry condition updates + refactoring
3 participants