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

Inclusion of ANN method #130

Closed
wants to merge 6 commits into from
Closed

Inclusion of ANN method #130

wants to merge 6 commits into from

Conversation

ggurioli
Copy link
Contributor

Inclusion of the ann.py, coding the class ANN for the definition of a basic ANN method.
Prove.pdf

ezyrb/ann.py Outdated

Example:
>>> import ezyrb
>>> import torch
Copy link
Contributor

Choose a reason for hiding this comment

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

These torch imports are not needed in the example

ezyrb/ann.py Show resolved Hide resolved
ezyrb/ann.py Outdated

def __init__(self):
self.trained_epoch = 0
# number of already trained iterations
Copy link
Contributor

Choose a reason for hiding this comment

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

the single comments go above the code line, not below

Copy link
Member

Choose a reason for hiding this comment

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

Moreover, I totally suggest to remove such comments, since the class variables should be properly explained in the class documentation. No need for duplication!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These comments have been ideated as a guideline. So maybe they could be useful if changes are needed by a new user.

Copy link
Member

Choose a reason for hiding this comment

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

That is the exact purpose of the documentation: helping new users. Please remove the comments.

ezyrb/ann.py Outdated Show resolved Hide resolved
ezyrb/ann.py Outdated Show resolved Hide resolved
ezyrb/ann.py Outdated Show resolved Hide resolved
ezyrb/ann.py Outdated
>>> import numpy as np
>>>
>>> x = np.random.uniform(-1, 1, size=(4, 2))
>>> from ann import ANN
Copy link
Member

Choose a reason for hiding this comment

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

Not tested, but it looks wrong. Have you check the example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I checked in the Prove.pdf file I attached in my "273cad3" commit. I report a demonstration below. (Out[14] is the training loss at termination).
Schermata 2021-02-23 alle 15 43 31

Copy link
Member

@ndem0 ndem0 Feb 23, 2021

Choose a reason for hiding this comment

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

Hope that the following convinces you: fresh installation of your commit, and tried the import you said be correct.
Take into account that testing the code means for all (or at least for the large majority) the environments, not only yours (or your directory, as in this case). Also please, check carefully the code, especially after someone's telling you there is probably a mistake.

(base) ~ [ python3 -m pip uninstall ezyrb
Found existing installation: ezyrb 1.0
Uninstalling ezyrb-1.0:
  Would remove:
    /data/local/lib/python3.8/site-packages/ezyrb-1.0-py3.8.egg
Proceed (y/n)? y
  Successfully uninstalled ezyrb-1.0

(base) ~ [ python3 -m pip install git+git://github.com/ggurioli/EZyRB.git@012ee67c4a05d8b26153b633f78e96a868258d16
Defaulting to user installation because normal site-packages is not writeable
Collecting git+git://github.com/ggurioli/EZyRB.git@012ee67c4a05d8b26153b633f78e96a868258d16
  Cloning git://github.com/ggurioli/EZyRB.git (to revision 012ee67c4a05d8b26153b633f78e96a868258d16) to /tmp/pip-req-build-ag4g3xrr
  Running command git clone -q git://github.com/ggurioli/EZyRB.git /tmp/pip-req-build-ag4g3xrr
Requirement already satisfied: future in /data/local/lib/python3.8/site-packages (from ezyrb==1.1) (0.18.2)
Requirement already satisfied: numpy in /usr/lib/python3.8/site-packages (from ezyrb==1.1) (1.18.4)
Requirement already satisfied: scipy in /usr/lib/python3.8/site-packages (from ezyrb==1.1) (1.4.1)
Requirement already satisfied: matplotlib in /usr/lib/python3.8/site-packages (from ezyrb==1.1) (3.2.1)
Requirement already satisfied: GPy in /data/local/lib/python3.8/site-packages (from ezyrb==1.1) (1.9.9)
Requirement already satisfied: cycler>=0.10 in /usr/lib/python3.8/site-packages (from matplotlib->ezyrb==1.1) (0.10.0)
Requirement already satisfied: kiwisolver>=1.0.1 in /usr/lib/python3.8/site-packages (from matplotlib->ezyrb==1.1) (1.1.0)
Requirement already satisfied: pyparsing!=2.0.4,!=2.1.2,!=2.1.6,>=2.0.1 in /usr/lib/python3.8/site-packages (from matplotlib->ezyrb==1.1) (2.4.7)
Requirement already satisfied: python-dateutil>=2.1 in /usr/lib/python3.8/site-packages (from matplotlib->ezyrb==1.1) (2.8.1)
Requirement already satisfied: paramz>=0.9.0 in /data/local/lib/python3.8/site-packages (from GPy->ezyrb==1.1) (0.9.5)
Requirement already satisfied: six in /usr/lib/python3.8/site-packages (from GPy->ezyrb==1.1) (1.14.0)
Requirement already satisfied: setuptools in /data/local/lib/python3.8/site-packages (from kiwisolver>=1.0.1->matplotlib->ezyrb==1.1) (46.1.3)
Requirement already satisfied: decorator>=4.0.10 in /usr/lib/python3.8/site-packages (from paramz>=0.9.0->GPy->ezyrb==1.1) (4.4.2)
Building wheels for collected packages: ezyrb
  Building wheel for ezyrb (setup.py) ... done
  Created wheel for ezyrb: filename=ezyrb-1.1-py3-none-any.whl size=12613 sha256=8efae9831e3b19c55bbd161a3b43e525307cab3882d2d6dd19bd014abc5e63a4
  Stored in directory: /data/.cache/pip/wheels/f3/91/7b/d288de7188c19541e855435b77de759352c0c3575ff48e7b92
Successfully built ezyrb
Installing collected packages: ezyrb
Successfully installed ezyrb-1.1

(base) ~ [ python3
Python 3.8.2 (default, Apr  8 2020, 14:31:25) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import ezyrb
>>> from ann import ANN
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'ann'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, now I understand what you mean. I've modified the init.py, installed my last commit, and tested the example as reported below. Is it fine?
Init
Terminal
Test

Copy link
Member

Choose a reason for hiding this comment

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

Now it seems reasonable!

ezyrb/ann.py Outdated
# loss values extraction (type: float)
loss.backward()
old_loss = loss.item()
Copy link
Member

Choose a reason for hiding this comment

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

It looks an unused variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be required for the subsequent self.optimizer.step(). I tried to remove "loss.backward()" but the training is not performing as expected (the magnitude of the training loss at termination is meaningful).

Copy link
Member

Choose a reason for hiding this comment

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

backward is for sure a fundamental step. I meant the old_loss variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, thanks.

ezyrb/ann.py Outdated
# update of the number of trained iterations
self.trained_epoch += niter
return loss.item()

def predict(self,new_point):
Copy link
Member

Choose a reason for hiding this comment

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

space after comma

@ndem0
Copy link
Member

ndem0 commented Feb 23, 2021

@ggurioli Can you implement also a minimal unittest module for the new class? (I suggest to trivially "translate" the test for GPR class https://github.com/mathLab/EZyRB/blob/master/tests/test_gpr.py)

ezyrb/ann.py Outdated
Example:
>>> import numpy as np
>>> from ann import ANN
>>> x = np.random.uniform(-1, 1, size = (4, 2))
Copy link
Contributor

Choose a reason for hiding this comment

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

size=(4, 2) no space here, since it's an argument.

@ndem0
Copy link
Member

ndem0 commented Feb 26, 2021

@ggurioli Several commits have similar names, which makes difficult navigate in the history. Please stash your commits (https://git-scm.com/book/it/v2/Git-Tools-Stashing-and-Cleaning) in order to clean the Git history.

@ggurioli ggurioli closed this Feb 26, 2021
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