API Updates for diffxpy
#146
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ilan-gold
force-pushed
the
ig/update_api
branch
from
bd92723 to
9f0886f
Compare
|
Just an update here - I added in the Poisson model as well. I think those 2 PR's (one per model) can be used for ease of review, but at the end of hte day, this is the branch with the ground truth of what we will merge. I needed one branch that has everything for testing. Hence the merge conflicts/Poisson model. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pairs with theislab/diffxpy#218
Our API is currently incompatible in a variety of ways with
diffxpy(not wildly surprising) in order to allow all of thediffxpytests to run. I have of course made some judgement calls here but the goal was to minimize breaking changes here and indiffxpy. There are a number of fixes as well for the normal model, which was outright incorrect.There is still the question (mentioned in slack) of whether the
jac_scaleis in general correct since it contains two multiplications byxh(especially since it seems we don't have a great way of testing since the jacobian is not used anywhere for anything).Also, the
mypyandsafetydo not appear to be working. Thesafetyis complaining about the dask version - we could loosen the constraint and fix it indiffxpywhere the issue appears to be so thatsafetydoesn't complain. The issue is linked in thepyproject.toml. For thesafety, I am not too sure what is going on here but I will try to figure it out - if you have any ideas, let me know! Very unfamiliar with this and the output is impossible to read for me!