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

(#Closes 2875) Implementation to handle elemental functions in wheres #2883

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

LonelyCat124
Copy link
Collaborator

Improves the implementation of wheres to do better with elemental functions. Its a relatively small change that should work out the box....

@LonelyCat124 LonelyCat124 marked this pull request as ready for review February 3, 2025 14:09
@LonelyCat124
Copy link
Collaborator Author

Gonna send this for integration

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.89%. Comparing base (06e0257) to head (f65a9f1).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2883   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files         359      359           
  Lines       50997    51069   +72     
=======================================
+ Hits        50942    51017   +75     
+ Misses         55       52    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LonelyCat124
Copy link
Collaborator Author

Ready for review assuming integration is ok @sergisiso @arporter

@LonelyCat124
Copy link
Collaborator Author

LonelyCat124 commented Feb 3, 2025

Integration passed successfully.

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

Thanks for this fix @LonelyCat124, as I mentioned in my in-line comments it is not obvious to me why this should work, but I suggest starting with my comments requesting additional tests as I will be ok with it if it is proven to work.

@LonelyCat124
Copy link
Collaborator Author

LonelyCat124 commented Feb 4, 2025

@sergisiso Comments should be addressed - had to reorder a couple of statments and add an additional test to directly test some code that is currently unreachable, but may be necessary to handle future improvements.

I will also rerun the integration tests as there are more impvoements here.

@LonelyCat124
Copy link
Collaborator Author

Previous branch had some deviation on nemo 5, will resubmit

@LonelyCat124
Copy link
Collaborator Author

Pushed the wrong branch to integration last night, set the right one going today.

@sergisiso
Copy link
Collaborator

Ah, the detail I has missing was that we don't support Imports or Unresolved anyway and therefore we don't have to worry about unknown 'elemental' attribute, it is much clearer now. I will wait until integration tests are done to finish the review.

@LonelyCat124
Copy link
Collaborator Author

Integration is now passing.

@sergisiso
Copy link
Collaborator

NEMOv5 CPU parallelisation was slower, but there were lots of people heavily using the machine that day and this is a threaded run, so it is expected that it was slower. But since this is one of our current targets, I want to re-run only this action when the machine is empty today or over the weekend.

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.

2 participants