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

Converting cinvertor python #1370

Merged
merged 119 commits into from
Apr 14, 2020
Merged

Converting cinvertor python #1370

merged 119 commits into from
Apr 14, 2020

Conversation

zattala
Copy link
Contributor

@zattala zattala commented Aug 9, 2019

Speed should be almost identical to C with Numba, without for the initial load about 2x/3x slower but for individual calculations still almost identical to C.

Calculated results the same with or without Numba.

Accuracy, has a e-15 difference on the final output, also because of truncated pi used in invertor.c the outputs will be slightly different, e-05, e-04 relative error, but with truncated pi used in calc.py should revert to default error of e-15.

Would also like more tests with smeared data if possible.

zattala and others added 30 commits July 1, 2019 12:06
both using njit() for now, tested multiple others with both, and not using map/lambdas using basic for for now. np.vectorize() and apply_along_axis() problem with njit()
@zattala
Copy link
Contributor Author

zattala commented Aug 15, 2019

I am not sure, but this may be the problem -
My initial change to setup.py not only removed the reference to c_extensions but kept a reference to a sascalc.pr.core, which does not appear to be in the current ESS_GUI branch, but must have been there when I first forked off of ESS_GUI as I did not add it, just removed c_extensions,
so original -
srcdir = os.path.join("src", "sas", "sascalc", "pr")
package_dir["sas.sascalc.pr.core"] = srcdir
package_dir["sas.sascalc.pr"] = os.path.join("src", "sas", "sascalc", "pr")
packages.extend(["sas.sascalc.pr", "sas.sascalc.pr.core"])

ESS_GUI -
srcdir = os.path.join("src", "sas", "sascalc", "pr", "c_extensions")
package_dir["sas.sascalc.pr.core"] = srcdir
package_dir["sas.sascalc.pr"] = os.path.join("src", "sas", "sascalc", "pr")
packages.append("sas.sascalc.pr")
ext_modules.append(Extension("sas.sascalc.pr._pr_inversion",
sources=[os.path.join(srcdir, "Cinvertor.c"),
os.path.join(srcdir, "invertor.c"),
],
include_dirs=[],
))

So removing the reference to both pr.core and the extension_modules reference should leave -
New:
package_dir["sas.sascalc.pr"] = os.path.join("src", "sas", "sascalc", "pr")
packages.append("sas.sascalc.pr")

Could this have caused the build to fail?

@rozyczko
Copy link
Member

The build failed at the code signing stage - something we are investigating currently. Your changes didn't cause the failure.

@rozyczko
Copy link
Member

5.0 ready for testing on Win

@rozyczko
Copy link
Member

The installer now gets created but the resulting installation (at least on windows) fails.
Will look at why later - most likely the pyinstaller script needs updating as well.

@rozyczko
Copy link
Member

5.0 ready for testing on Win

@rozyczko
Copy link
Member

5.0 ready for testing on Win

@zattala
Copy link
Contributor Author

zattala commented Aug 20, 2019

sorry to commit again after the build has been tested but found small bug, just method being called wrong, when testing pr inversion with smearing. It does work properly now though.

@pkienzle
Copy link
Contributor

Before merging, we need ortho_transformed in calc.py to use sinc function so that the calculation works when q is zero. There are other review comments, but these are for style and (minor) efficiency gains, not for bug fixes.

Also, can't merge because of conflicts.

@pkienzle
Copy link
Contributor

Ready for testing on Win64

@butlerpd
Copy link
Member

butlerpd commented Oct 1, 2019

the 4.x version of this PR (#1038 ) was merged on October 1. As agreed this PR should NOT be merged for 5.01 but leaving it here to remind us that we need to eventually bring it into 5.x

@butlerpd
Copy link
Member

One unit test is failing - 1.0000.....2 !=1 . That seems like a rounding error? Perhaps it should be an AssertAlmostEqual?

@butlerpd
Copy link
Member

The tests are all now passing and as per agreement on Tuesday calls going back a while this is now ready to merge.

Question for @wpotrzebowski or @rozyczko; given the plan for a 5.0.2 should we hold off for this week? or shall we just dump it in? the 4x version got merged a while back with no problems yet. One caveat: we really don't want to wait longer than this week or we will once again have to worry about keeping this branch up to date and yada yada -- trying desperately to clear our PR backlog.

@butlerpd butlerpd merged commit d381d7a into ESS_GUI Apr 14, 2020
@krzywon krzywon deleted the converting_cinvertor_python branch May 29, 2020 18:58
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.

4 participants