https://github.com/ziatdinovmax/pyroVED/issues/54

[utkarshp1161]

Based on Discussion:
#53

[ziatdinovmax]

A few comments/suggestions:

• Please add the unit tests to cover new code lines
• I suggest moving GP to a separate utility function and importing it from there. Then you just call something like gpr(*args, **kwargs).
• What is the meaning of training_data? Is it the same as the one used to train VAE? Or can it be any X? Need to explain it clearly.
• Making predictions on a z grid without any connection to input space may not be very useful. Let's add a decoder part that will show how these z-points look in the original data space.

[ziatdinovmax]

@utkarshp1161 Any progress on this one?

[utkarshp1161]

Hi Max,

Was busy with qualifiers this past week. Will get on this.

[utkarshp1161]

Hi Max,

1. https://github.com/utkarshp1161/pyroVED/blob/main/pyroved/utils/gp.py has the gp model
2. https://github.com/utkarshp1161/pyroVED/blob/main/pyroved/models/ivae.py has the incorporated def predict_on_latent() funtion
3. Notebook used: https://github.com/utkarshp1161/pyroVED/blob/main/examples/VAE_gp.ipynb

wrt tests:

I am getting below error when I run: pytest test/:
see error.txt file attached
error.txt

I am thinking of adding tests similar to manifold2d test in test_model.py. Also for testing gp part I am thinking of
adding a test_utils.py in https://github.com/utkarshp1161/pyroVED/blob/main/tests/

Do you have suggestions on what to include in tests?

[utkarshp1161]

Hi Max,

1. https://github.com/utkarshp1161/pyroVED/blob/main/pyroved/utils/gp.py has the gp model
2. https://github.com/utkarshp1161/pyroVED/blob/main/pyroved/models/ivae.py has the incorporated def predict_on_latent() funtion
3. Notebook used: https://github.com/utkarshp1161/pyroVED/blob/main/examples/VAE_gp.ipynb

wrt tests:

I am getting below error when I run: pytest test/: see error.txt file attached error.txt

EDIT: resolved: Environment was pointing to local python env rather than conda env. pytest running but below error
error2.txt

I am thinking of adding tests similar to manifold2d test in test_model.py. Also for testing gp part I am thinking of adding a test_utils.py in https://github.com/utkarshp1161/pyroVED/blob/main/tests/

Do you have suggestions on what to include in tests?

[ziatdinovmax]

Thanks for the update/revision.

Several comments:

• In ivae.py, on line 329, you seem to be overwriting the gp_iterations parameter and setting it to 1.
• In the utils/gp.py, the docstring says that you define and train a GP model, but there's no actual training.
• I suggest that the predict_on_latent method returns both the GP predictions and the manifold2d tensor, whole the plotting is optional.
• For tests, just add a separate test for the (modified) GP utility to confirm that it runs and returns a tensor with an expected shape. Then, do the same for the predict_on_latent.

[utkarshp1161]

Thanks for the update/revision.

Several comments:

• In ivae.py, on line 329, you seem to be overwriting the gp_iterations parameter and setting it to 1.

Thanks for pointing out.

• In the utils/gp.py, the docstring says that you define and train a GP model, but there's no actual training.

Yep. Changed

• I suggest that the predict_on_latent method returns both the GP predictions and the manifold2d tensor, whole the plotting is optional.

Added a "plot" boolean which defaults to False. Returns GP predictions and the manifold2d tensor

• For tests, just add a separate test for the (modified) GP utility to confirm that it runs and returns a tensor with an expected shape. Then, do the same for the predict_on_latent.

Yep added.

Thanks for reviewing. Please let me know further changes as necessary.

[ziatdinovmax]

Almost there... Instead of returning z, we should be returning z_decoded, which is z passed through a trained decoder. In this particular case, we can obtain it simply as z_decoded = self.manifold2d(d, plot=False).

I'm curious if there's a specific reason you don't want to train GP inside utils/gp.py?

[utkarshp1161]

Almost there... Instead of returning z, we should be returning z_decoded, which is z passed through a trained decoder. In this particular case, we can obtain it simply as z_decoded = self.manifold2d(d, plot=False).

I was having something else in mind, basically having the latent coordinates rather than decoded ones. Changed as per this suggestion.

I'm curious if there's a specific reason you don't want to train GP inside utils/gp.py?

Changed training to utils. Right, this way its more modular.

[ziatdinovmax]

I was having something else in mind, basically having the latent coordinates rather than decoded ones. Changed as per this suggestion.

In principle, we can have all three. Something like

return (z, z_decoded), predictions

Let me know if you would like to add this. Other than that I'm ready to merge it.

[codecov-commenter]

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (ce84746) 91.60% compared to head (ce0b58d) 91.33%.

Files	Patch %	Lines
pyroved/models/ivae.py	58.62%	12 Missing ⚠️
tests/test_models.py	94.11%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
- Coverage   91.60%   91.33%   -0.28%     
==========================================
  Files          25       27       +2     
  Lines        1918     1996      +78     
==========================================
+ Hits         1757     1823      +66     
- Misses        161      173      +12     

Flag	Coverage Δ
unittests	91.33% <83.54%> (-0.28%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.