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

Rename earth_model to detector_model #43

Merged
merged 28 commits into from
Jan 14, 2024
Merged

Rename earth_model to detector_model #43

merged 28 commits into from
Jan 14, 2024

Conversation

austinschneider
Copy link
Collaborator

Just realized we forgot to rename these variables in the previous PR

@austinschneider austinschneider added the enhancement New feature or request label Jan 11, 2024
Copy link
Collaborator

@nickkamp1 nickkamp1 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, just need to change the earthparams directory to DetectorParams. I will do this in ~30 min after biking into work :)

projects/detector/private/DetectorModel.cxx Show resolved Hide resolved
projects/injection/private/pybindings/LeptonInjector.cxx Outdated Show resolved Hide resolved
@austinschneider
Copy link
Collaborator Author

Looks like there are a few other places that "earth" pops in that we might want to rename.

@austinschneider
Copy link
Collaborator Author

There is also a file in math called Coordinates.cxx that we can delete.

@nickkamp1
Copy link
Collaborator

Looks like there are a few other places that "earth" pops in that we might want to rename.

Yeah, for example functions like GetDetCoordPosFromEarthCoordPos should probably be renamed, but it might make sense to do that on the coordinate transformation branch?

@austinschneider
Copy link
Collaborator Author

Finally, something is causing the build to fail pretty early on in the GitHub runners. We should fix that too.

@nickkamp1
Copy link
Collaborator

There is also a file in math called Coordinates.cxx that we can delete.

I agree it seems to be legacy. But I think we should delete it in the coordinate transformation branch?

@nickkamp1
Copy link
Collaborator

Looks like there are a few other places that "earth" pops in that we might want to rename.

Yeah, for example functions like GetDetCoordPosFromEarthCoordPos should probably be renamed, but it might make sense to do that on the coordinate transformation branch?

Ok I actually went back and renamed every earth -> detector instance that doesn't interface with DetectorModel::GetEarthCoordPosFromDetCoordPos, since I think we should rename that function based on the updates in the coordinate transformation branch

@nickkamp1
Copy link
Collaborator

Finally, something is causing the build to fail pretty early on in the GitHub runners. We should fix that too.

I think this is the last remaining issue on this branch before we can merge

@austinschneider
Copy link
Collaborator Author

Cool, yeah I'll make those changes in the coordinate transformation branch 👍

@austinschneider
Copy link
Collaborator Author

These latest cmake changes allowed one of the linux builds to work. We'll see if this extends to the rest of the builds invoked by cibuildwheel 🤞

@austinschneider
Copy link
Collaborator Author

The linux builds seem to be going just fine now, but the macos builds are still stuck on the delocate stage. It seems like it is complaining that it cannot find python. If it is trying to package the python shared object library into the wheel, then we could stop it from doing so with --ignore-missing-dependencies, but it may actually need this. In any case, it is strange that we're getting this error. It is complaining that /Library/Frameworks/Python.framework/Versions/3.8/Python is not found, but this file does exist and is the correct python library.

@austinschneider
Copy link
Collaborator Author

austinschneider commented Jan 13, 2024

Okay, we're still getting the same error on cd2f3a4. I forgot that the Development.Module was conditional on cibuildwheel, so trying the combination of Development.Module and pybind11::python_link_helper on my local machine actually has the same linking error with python.

@austinschneider
Copy link
Collaborator Author

austinschneider commented Jan 14, 2024

At least one of the macos builds is finishing now!

@nickkamp1
Copy link
Collaborator

At least one of the macos builds is finishing now!

awesome thanks Austin!!

@austinschneider
Copy link
Collaborator Author

@nickkamp1 we can go ahead and merge this once you have a chance to look things over!

Copy link
Collaborator

@nickkamp1 nickkamp1 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, merge approved! Thanks for getting the ci build working Austin

@nickkamp1 nickkamp1 merged commit 312efdc into main Jan 14, 2024
14 checks passed
@nickkamp1 nickkamp1 deleted the dev/renaming branch January 14, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants