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

tests for maml and opti #38

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

Riddhi2587
Copy link

No description provided.

Copy link
Member

@abhi-glitchhg abhi-glitchhg left a comment

Choose a reason for hiding this comment

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

Hey @Riddhi2587, I am sorry for poking in the draft pr.

I have added some comments; please go through them. I will do another review after these comments are addressed.

If you have made these changes in the local repository and haven't updated the remote branch, ignore these changes.

Comment on lines +14 to +19
# self.assertIsInstance(params,)
self.assertIsInstance(apply_fn, Callable)
self.assertIsInstance(adapt_fn, Callable)
self.assertIsInstance(loss_fn, Callable)
state = Opti.create(params, apply_fn, adapt_fn, loss_fn, tx)
self.assertIsInstance(state, MetaTrainState)
Copy link
Member

Choose a reason for hiding this comment

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

What you are doing here is very thoughtful and is in the right direction. But the problem with this approach is as the repo gets bigger, writing these assert statements will be very painful. So how about automating all this?

There exists a tool for doing this called mypy. Mypy is a static type checker tool , and as we are including type hints, it makes sense to add mypy tool in repo.

Copy link
Member

Choose a reason for hiding this comment

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

I can set up mypy for the repository which will do type-checking tasks on the library and on the test modules.

looping in @veds12

Comment on lines +24 to +27
self.assertIsInstance(state, MetaTrainState)
self.assertIsInstance(metrics, List[Callable[[ndarray, ndarray], ndarray]])
meta_train = Opti.meta_train_step(state, tasks, metrics)
self.assertIsInstance(meta_train, Tuple[MetaTrainState, ndarray, List[ndarray]])
Copy link
Member

Choose a reason for hiding this comment

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

same applies here.

Comment on lines +4 to +7

sys.path.append('C:users/riddh/git-repos/jeta/jeta')
# from opti_trainer import *
import opti_trainer as opt
Copy link
Member

Choose a reason for hiding this comment

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

clever hack! but this will only work on your local system.

init.py is empty, this is why you are not able to import opti_trainer and other functions/ class here.

You can only import opti_trainer if the above init file has an import statement for opti_trainer.

So by adding following line in init.py,

from .opti_trainer import OptiTrainer

there will be no need to write this: sys.path.append('C:users/riddh/git-repos/jeta/jeta') .

then to import the OptiTrainer class in the tests module, use

from jeta import OptiTrainer

Comment on lines +2 to +4

sys.path.append('C:users/riddh/git-repos/jeta/jeta')
import maml
Copy link
Member

Choose a reason for hiding this comment

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

same here as well.

@abhi-glitchhg
Copy link
Member

@Riddhi2587 Any updates?

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.

2 participants