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

Multiple Inputs: update the Model module #610

Open
wants to merge 123 commits into
base: main
Choose a base branch
from
Open

Conversation

amal-ghamdi
Copy link
Contributor

@amal-ghamdi amal-ghamdi commented Jan 17, 2025

closes #605

Notes about reviewing: see issue description

@amal-ghamdi amal-ghamdi requested review from nabriis and chaozg February 2, 2025 13:40
Copy link
Collaborator

@nabriis nabriis left a comment

Choose a reason for hiding this comment

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

From live review of code

  1. Printing looks a bit strange for Model. Maybe use specific case for ProductGeometry?
  2. Can you check that if we pass in CUQIArray but say its funval/parameters what happens? Works as intended?
  3. Related to stacking. Do we have good docstring to explain this option? Should we ask for user to provide e.g. "is_stacked".
  4. What about Samples in the Dict? Why is the stacked samples not support?
  5. Check that given RVs x, y, z, that F(x, y, z) works.
  6. Look into gradient as None. Do we need it now? Seems to change behavior. Maybe we return error?
  7. Gradient as stacked. Look into "is_stacked" parameter.
  8. Look into stacked output.
  9. Look into if Model should be two classes or one?

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.

Multiple Inputs: Update forward model to accept multiple inputs
2 participants