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

Switch from association to link #200

Merged
merged 5 commits into from
Sep 9, 2024
Merged

Switch from association to link #200

merged 5 commits into from
Sep 9, 2024

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Aug 23, 2024

BEGINRELEASENOTES

  • Switch from Association to Link in the conversion from EDM4hep to LCIO to accomodate for the new naming from (EDM4hep#341)
  • Add test to make sure that conversion of Links from EDM4hep to LCIO works as expected

ENDRELEASENOTES

Fixes #199 (I think). @andresailer was this what you were referring to in that issue?
Fixes key4hep/k4EDM4hep2LcioConv#93


for (const auto& [edm4hepName, lcioName] : collsToConvert) {
const auto coll = getEDM4hepCollection(edm4hepName);
if (coll->getTypeName().find("Association") != std::string_view::npos) {
Copy link
Member

Choose a reason for hiding this comment

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

This is what #199 is referring to since now it's impossible to find "Association" in the type name.

@jmcarcell jmcarcell changed the title Switch from assocation to link Switch from association to link Aug 24, 2024
@tmadlener
Copy link
Contributor Author

Seems, I didn't merge this yet. AFAIU this solves the issue and can be merged, right?

@jmcarcell
Copy link
Member

Maybe there should be a test doing something with links? Otherwise this can just not be caught if there is another problem like this one.

@tmadlener
Copy link
Contributor Author

There is already a test with links global_converter_maps that goes from LCIO to EDM4hep (which works without problems apparently). I added a new test that also checks the other direction. I added this as a completely separate test that uses one new Transformer for creating the Links in EDM4hep and then it uses another new Marlin Processor for checking it's contents. Prior to the fixes this test fails, so it does indeed check that links are converted.

@tmadlener
Copy link
Contributor Author

@jmcarcell do you want to have another closer look? Otherwise, I would merge this later today.

std::to_string(i) + ", actual: " + std::to_string(rel->getWeight()) + ")");
}

if (!(rel->getTo() == mc)) {
Copy link
Member

Choose a reason for hiding this comment

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

So we don't have !=?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do, especially because for LCIO this is just comparing pointers. (We would also have it for EDM4hep). This is a remnant from copy-pasting.

@tmadlener tmadlener merged commit 8e0eb80 into main Sep 9, 2024
9 of 11 checks passed
@tmadlener tmadlener deleted the switch-assoc-link branch September 9, 2024 08:14
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.

Some conversions are missing from edm4hep to lcio? Fix Association checks
2 participants