Skip to content

add orb support... tricky #303

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

Merged
merged 9 commits into from
Feb 19, 2025
Merged

add orb support... tricky #303

merged 9 commits into from
Feb 19, 2025

Conversation

alinelena
Copy link
Member

No description provided.

@alinelena alinelena added the enhancement New/improved feature or request label Sep 4, 2024
@alinelena alinelena self-assigned this Sep 4, 2024
@alinelena alinelena marked this pull request as draft September 4, 2024 10:22
Copy link
Collaborator

@oerc0122 oerc0122 left a comment

Choose a reason for hiding this comment

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

Could be simplified. Depends if you think it's more or less readable.

@ElliottKasoar
Copy link
Member

Mostly for reference but just to note this may pin torch to 2.2.0, which at the moment is fine, but could be a problem in the future.

(I need to double check but I think even uninstalled extras' dependencies are taken into account when installing)

@ElliottKasoar
Copy link
Member

ElliottKasoar commented Jan 2, 2025

I think the installation is all working now with uv, but since I don't think we're dropping 3.9 quite yet, the match statement still needs changing.

The README and getting started lists should also be updated.

@alinelena alinelena marked this pull request as ready for review February 14, 2025 18:02
Copy link
Member

@ElliottKasoar ElliottKasoar left a comment

Choose a reason for hiding this comment

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

I think we want a test for the calculator init e.g. parameters are set, and (invalid) model path works as expected, but generally looks good!

@ElliottKasoar
Copy link
Member

ElliottKasoar commented Feb 17, 2025

Rebased, and updated docs. Setting the model_path needs fixing, and then a couple more tests to be added.

Then the MacOS dm-tree error needs fixing.

@alinelena
Copy link
Member Author

Rebased, and updated docs. Setting the model_path needs fixing, and then a couple more tests to be added.

Then the MacOS dm-tree error needs fixing.

i wonder if we shall to a try catch... and then raise accordingly?

@ElliottKasoar ElliottKasoar force-pushed the orb branch 2 times, most recently from 01dea3c to 480a391 Compare February 18, 2025 17:08
@ElliottKasoar
Copy link
Member

It's possible there are slightly neater versions for setting the model, but I think it now uses the default model correctly, as well as using the pretrained model labels, or a loaded model (which is tested with their XS checkpoint file).

Also installed cmake on our Mac, so hopefully those tests are fixed.

Copy link
Member

@ElliottKasoar ElliottKasoar left a comment

Choose a reason for hiding this comment

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

I can't officially approve, but I think it looks good

Copy link
Collaborator

@harveydevereux harveydevereux left a comment

Choose a reason for hiding this comment

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

Also looks good to me

@ElliottKasoar ElliottKasoar merged commit 485084e into stfc:main Feb 19, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New/improved feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add orb support
4 participants