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

Update fastpfor source and header files to incorporate changes made since 2018. #2

Merged
merged 2 commits into from
Oct 14, 2023

Conversation

jsford
Copy link
Contributor

@jsford jsford commented Jul 13, 2023

Hello @searchivarius,

Please consider this pull request to update the source and header files for fastpfor. I have copied in the source and header files from the most recent fastpfor commit --- 8e67fbd3418e27c8b593cc94335fc584c5e937cb --- made on June 5 2023.

I am motivated to update these files because I am using both PyFastPFor and lemire/FastPFor in my projects, and I would like the results to match. There have been small tweaks to the encodings made since your last checkpoint in 2018, so I am currently unable to use PyFastPFor to decode files I have written using FastPFor.

I was able to swap in the new files without too many problems. The only substantive change I made to the code is to instantiate a CODECFactory in the IntegerCODECWrapper since the formerly static methods getFromName() and allNames() now require an instance of the factory to call.

If you're willing to accept this pull request, that would be awesome. If you'd like to add lemire/FastPFor as a submodule, I'd be happy to set that up as well.

Regards,
Jordan Ford

@searchivarius
Copy link
Owner

This is great! @jsford could we have some at least basic test to check if things are working?

@jsford
Copy link
Contributor Author

jsford commented Jul 14, 2023

Cool, sure. I ran the test_basic1.py script on my machine without errors. What additional tests did you have in mind?

@searchivarius
Copy link
Owner

@jsford are you submitting test_basic as a part of pull request? You can add tests to setup.py.

@jsford
Copy link
Contributor Author

jsford commented Jul 15, 2023

@searchivarius --- this confused me, but I think I figured out the issue.

test_basic1.py already existed in your repo. My pull request doesn't list the test_basic1.py file because I didn't need to make any changes to it. As far as I can tell, you should be able to accept this pull request and continue to use the pre-existing test script that is already in your repository.

I've actually never submitted a pull request before, so if I've got this all wrong, just let me know!

@searchivarius
Copy link
Owner

@jsford sorry about the confusion. Indeed, there's a test.

Did you add new codecs too? The test is only for a bunch of old ones.

@searchivarius
Copy link
Owner

Hi @jsford please see my question above!

…, and improve the formatting of the test output.
@jsford
Copy link
Contributor Author

jsford commented Aug 19, 2023

Hi @searchivarius
I've updated the test script to cover all codecs returned by getCodecList(). I also made some changes to the formatting of the test output, and I changed the filename from test_basic1.py to test_codecs.py. Let me know what you think!

@searchivarius
Copy link
Owner

@jsford sorry about delays. This is awesome, I will look into merging the PR soon!

@jsford
Copy link
Contributor Author

jsford commented Sep 22, 2023

Hi @searchivarius, just a reminder to look into this. Thanks!

@searchivarius searchivarius changed the base branch from master to dev October 14, 2023 03:32
@searchivarius searchivarius merged commit b989fb8 into searchivarius:dev Oct 14, 2023
@searchivarius
Copy link
Owner

Hi @jsford : just being overloaded, sorry.

Anyways, it all seems to be working (all tests pass) and the new version is uploaded to PyPi. I think buiding needs an update potentially, std=C++-11 flag doesn't work with MAC OS (despite there's a check in setup.py). However, it's possible to install on MAC with some hacks, and it's just fine on Linux.

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.

None yet

2 participants