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

Fix: import of private symbols affects the type inference #308

Merged
merged 3 commits into from
May 7, 2024

Conversation

quepas
Copy link
Contributor

@quepas quepas commented May 2, 2024

As described in #309, imports of private symbols destroys the type inference. This pull request propose a simple check of symbols access specificators to filter out the private symbols.

TODO:

  • Support for other frontends
  • Unit test

@quepas quepas changed the title Bug: import of private symbols affects the type inferece Fix: import of private symbols affects the type inferece May 2, 2024
@quepas quepas force-pushed the no_import_private_symbols branch 2 times, most recently from 4b456b1 to 2b55a60 Compare May 2, 2024 14:16
@reuterbal reuterbal linked an issue May 3, 2024 that may be closed by this pull request
@quepas quepas force-pushed the no_import_private_symbols branch from 7cb154d to c5488e6 Compare May 3, 2024 11:25
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Many thanks for finding this and providing the fix, this looks fantastic!

I've just a few stylistic pointers but otherwise very happy to merge this as soon as possible.

loki/frontend/tests/test_frontends.py Outdated Show resolved Hide resolved
loki/frontend/tests/test_frontends.py Outdated Show resolved Hide resolved
loki/frontend/omni.py Outdated Show resolved Hide resolved
loki/frontend/ofp.py Outdated Show resolved Hide resolved
loki/frontend/fparser.py Outdated Show resolved Hide resolved
@reuterbal reuterbal changed the title Fix: import of private symbols affects the type inferece Fix: import of private symbols affects the type inference May 3, 2024
Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.09%. Comparing base (dd01f94) to head (9b8ac61).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #308   +/-   ##
=======================================
  Coverage   95.09%   95.09%           
=======================================
  Files         165      165           
  Lines       35282    35301   +19     
=======================================
+ Hits        33552    33571   +19     
  Misses       1730     1730           
Flag Coverage Δ
lint_rules 96.39% <ø> (ø)
loki 95.07% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@quepas quepas force-pushed the no_import_private_symbols branch from c5488e6 to f6d98b7 Compare May 3, 2024 16:20
@quepas quepas force-pushed the no_import_private_symbols branch from f6d98b7 to 9b8ac61 Compare May 3, 2024 16:22
@quepas
Copy link
Contributor Author

quepas commented May 3, 2024

Many thanks for finding this and providing the fix, this looks fantastic!

I've just a few stylistic pointers but otherwise very happy to merge this as soon as possible.

Thank you for great suggestions! I have applied them.

No problem, the pleasure is on our side (IPSL), as we are actively using loki in our work.

@quepas quepas requested a review from reuterbal May 4, 2024 20:11
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Many thanks for implementing the suggestions. This looks really great now!

(At some point I'd be curious to learn some details about your work at IPSL if you're willing to share, e.g., by email).

@reuterbal reuterbal added the ready to merge This PR has been approved and is ready to be merged label May 6, 2024
Copy link
Collaborator

@mlange05 mlange05 left a comment

Choose a reason for hiding this comment

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

Fully agree, this looks great. Many thanks @quepas

@reuterbal
Copy link
Collaborator

ecwam regression fails due to missing permissions from an external contribution. Verified compatibility offline.

@reuterbal reuterbal merged commit 5db17e5 into ecmwf-ifs:main May 7, 2024
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR has been approved and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: import of private symbols affects the type inferece
3 participants