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

Refactor: Handling Unrecognized Arguments for Prepare #696

Merged
merged 45 commits into from
Jul 3, 2024

Conversation

selmanozleyen
Copy link
Collaborator

@selmanozleyen selmanozleyen commented May 21, 2024

These are the things I want to do first. related: #596

From reading through ottjax only the __call__ functions on solvers absorb kwargs that aren't recognized. And they are because either they are given to create_geometry methods or create_initializer methods:

So this is kind of good news because from what I read from moscot, call_fn arguments are filtered before here:

@classmethod
def _partition_kwargs(cls, **kwargs: Any) -> Tuple[Dict[str, Any], Dict[str, Any]]:
"""Partition keyword arguments.
Used by the :meth:`~moscot.problems.base.BaseProblem.solve`.
Parameters
----------
kwargs
Keyword arguments to partition.
Returns
-------
Keyword arguments for :class:`BaseSolver` and :meth:`__call__`, respectively.
"""
call_kws, shared_kws = cls._call_kwargs()
init_kwargs = {k: v for k, v in kwargs.items() if k not in call_kws or k in shared_kws}
call_kwargs = {k: v for k, v in kwargs.items() if k in call_kws or k in shared_kws}
return init_kwargs, call_kwargs

Overall this means:

  • We can fix theprepare methods absorbing kwargs ourselves in moscot, by simply removing the usage of kwargs in concrete classes, so I will dedicate this PR to it
  • But for solve we will need to refactor things from ottjax. It won't be too hard and I already spotted where they are being ignored. If we refactor ottjax we might not need any changes in moscot since the error will be thrown from there.

Related issues (will be closed after merge):

I separated some issues from this PR so that we can work on them later:

Now that the tests pass these are the things to do:

  • clean up code (most likely requires refactoring of handle_joint_attr)
  • update docstrings
  • clean the TODO's and ensure that everyone agrees on the new docs and docstrings
  • write tests to ensure it fails (not sure if tests are necessary at this point since the checks are static now, will probably figure out cases to test when working on Refactor: Check if both x and x_callback is given #715)

@selmanozleyen selmanozleyen changed the title Refactor: Handling Unrecognized Arguments Refactor: Handling Unrecognized Arguments for Prepare May 27, 2024
Copy link
Member

@giovp giovp left a comment

Choose a reason for hiding this comment

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

fantastic work @selmanozleyen !

src/moscot/problems/space/_alignment.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@MUCDK MUCDK left a comment

Choose a reason for hiding this comment

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

Really great work Selman! Thanks so much!

@selmanozleyen
Copy link
Collaborator Author

@MUCDK the problem is even though 3.10 passes 3.9 now fails. The results change between these versions

@MUCDK
Copy link
Collaborator

MUCDK commented Jul 3, 2024

Ok can we skip the test for python 3.9?

@selmanozleyen
Copy link
Collaborator Author

Sure will update the tests in a new PR then

@selmanozleyen selmanozleyen merged commit 5706ec0 into main Jul 3, 2024
5 of 8 checks passed
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.

3 participants