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

Add support for negation to f64 #34

Merged
merged 7 commits into from
Jul 18, 2023
Merged

Conversation

wbthomason
Copy link
Contributor

Now that upstream egglog supports neg for f64 (egraphs-good/egglog#168), this PR adds support to the f64 sort in these bindings.

Closes #32

@wbthomason
Copy link
Contributor Author

This PR does require updating https://github.com/saulshanabrook/egg-smol first, I think?

@saulshanabrook
Copy link
Member

Yeah I am keeping an upstream fork currently to pull in the visualization work which isn't merged yet (egraphs-good/egglog#147).

I just merged in the recent commits, including yours, to that branch. If you update the pinned commit to 353c4387640019bd2066991ee0488dc6d5c54168 it should include your changes.

If you want to add a small test to test_high_level.py or an example file for floats, then you can verify that.

@wbthomason
Copy link
Contributor Author

Thanks! I've made these changes - but I'm hitting a persistent error (ImportError: Error importing plugin "mypy.test.data": No module named 'mypy.test') running pytest, even after running pip install -e .[dev,test]. Is there possibly a dependency missing?

Copy link
Member

@saulshanabrook saulshanabrook left a comment

Choose a reason for hiding this comment

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

I left a comment about removing some lines from the test file to help it pass...

I am not sure about the pytest import error! Could you post the full traceback from the command? What OS are you running with as well?

Also if you wouldn't mind adding an entry to the changelog that would be great!

python/tests/test_high_level.py Outdated Show resolved Hide resolved
@saulshanabrook saulshanabrook enabled auto-merge July 18, 2023 16:59
@wbthomason
Copy link
Contributor Author

I left a comment about removing some lines from the test file to help it pass...

Done, thanks for the pointer!

Also if you wouldn't mind adding an entry to the changelog that would be great!

Also done!

I am not sure about the pytest import error! Could you post the full traceback from the command? What OS are you running with as well?

I fear this may have just been a transient bug with my Python virtualenv setup? Activating the env in a fresh terminal allows pytest to work as expected. ¯\(ツ)

@saulshanabrook
Copy link
Member

saulshanabrook commented Jul 18, 2023

Looks like the only remaining failure is with Python 3.8. Rust is failing to compile egglog, due to using some unstable rust feature:

      error[E0658]: use of unstable library feature 'is_some_and'
          --> /home/runner/.cargo/git/checkouts/egg-smol-5b6bf12ac6ac9a68/353c438/src/lib.rs:1033:42
           |
      1033 |                 if self.get_sort(&value).is_some_and(|sort| sort.is_eq_sort()) {
           |                                          ^^^^^^^^^^^
           |
           = note: see issue #93050 <https://github.com/rust-lang/rust/issues/93050> for more information

It looks like this feature was added to rust 1.70.0 rust-lang/rust#93050 (comment)

I am not sure why the other python version builds work fine?

EDIT: It looks like the 3.9 test used a newer runner image:

  Image: ubuntu-22.04
  Version: 20230716.1.0

than the 3.8 run:

  Image: ubuntu-22.04
  Version: 20230517.1

I will try re-running the 3.8 manually.

@saulshanabrook saulshanabrook merged commit b05a79b into egraphs-good:main Jul 18, 2023
@saulshanabrook
Copy link
Member

Thank you for adding this and being the first external contributor!

I just made a release so this will be in v0.5.1.

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.

Question: negation for f64
2 participants