search

mirapy-org / mirapy

MiraPy: A Python package for Deep Learning in Astronomy
https://mirapy.org/
MIT License
44 stars 10 forks source link

Added XRayBinaryClassifier model #12

Closed akhilsinghal1234 closed 5 years ago

akhilsinghal1234 commented 5 years ago

Unit testing to be done

swapsha96 commented 5 years ago

I see a fit function is missing and optimizer is not declared. I will suggest

  1. Adding fit function that takesXandy` as input and returning output.
  2. Taking optimizer as an input in __init__ with default value as None. In fit function if self.optimizer is None, Adam optimizer is used for the same.
swapsha96 commented 5 years ago

Also,

  1. Rename classifier.py to classifiers.py. Do the same with model.py and rename it to models.py.
swapsha96 commented 5 years ago

Could you put an example code of this class here? We'll put it in the docs later.

pep8speaks commented 5 years ago

Hello @akhilsinghal1234! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2019-05-01 17:53:23 UTC
akhilsinghal1234 commented 5 years ago

Instead of adding optimizer and activation in init, shouldn't they be in build model?

swapsha96 commented 5 years ago

Oh yes. You are right. I guess then it's better to call that function compile rather than build model. (I saw another example where they used that name).

swapsha96 commented 5 years ago

Also, looks at the comments by @pep8speaks above. It refreshes after every commit. Very useful!

swapsha96 commented 5 years ago

One small change is left (see above). Also, since the dataset is small, data the files in a folder in data module along with a loader function.

akhilsinghal1234 commented 5 years ago

Please comment any changes needed. Added train.py and utils.py too.

swapsha96 commented 5 years ago

Please change the location of dataset from mirapy/dataset/XRayBinary/Dataset/ to mirapy/dataset/XRayBinary/ and set is as the default value of data_directory in load_dataset.

Also, I have added load_dataset.py in data module. Remind me to move load_dataset function in utils.py to that script (maybe calling the function load_XRB_binary_dataset.

Also, resolve add PEP8 issues mentioned above by @pep8speaks.

swapsha96 commented 5 years ago

LGTM!