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

Issues with ValidateKCSD #52

Open
ccluri opened this issue Sep 10, 2018 · 4 comments
Open

Issues with ValidateKCSD #52

ccluri opened this issue Sep 10, 2018 · 4 comments
Assignees

Comments

@ccluri
Copy link
Collaborator

ccluri commented Sep 10, 2018

  1. Inconsistent documentation of the class 'nr_basis' for instance. 'mask', 'kcsd_xlims', is not mentioned. 'ele_xlims' is in the documentation but not in the kwargs pops / also only self.ele_lims seems to be used.
  2. Why are the remaining parameters passed in the make_reconstruction function as opposed to the original function - what is the justification this separation?
  3. Whats with the csd_seed being passed in make_reconstruction and also in the class init for ValidateKCSD1D, 2D, 3D etc? Please consider writing them as separate functions if you want to update them. Like def update_csd_seed(): , def update_ele_lims()., csd_profile is another. It is confusing to pass these values twice.
  4. consider changing the name of function 'recon' - its similar to make_reconstruction - and both call the KCSD classes in the end.

This class needs more code review.

@ccluri
Copy link
Collaborator Author

ccluri commented Sep 10, 2018

Ideally, in https://github.com/Neuroinflab/kCSD-python/blob/bc0dc3d1c61ddf27ffb9a7bdd85ac4e627432a8f/tutorials/tutorial_basic.ipynb

The last notebook 'Cell' - should be all of the tutorial before - but passed in one or two simple lines - perhaps this is already do-able, its just that it is misleading with the documentation

m-kowalska added a commit to m-kowalska/kCSD-python that referenced this issue Nov 15, 2018
m-kowalska added a commit to m-kowalska/kCSD-python that referenced this issue Nov 16, 2018
@m-kowalska
Copy link
Collaborator

Ideally, in https://github.com/Neuroinflab/kCSD-python/blob/bc0dc3d1c61ddf27ffb9a7bdd85ac4e627432a8f/tutorials/tutorial_basic.ipynb

The last notebook 'Cell' - should be all of the tutorial before - but passed in one or two simple lines - perhaps this is already do-able, its just that it is misleading with the documentation

@ccluri This is already implemented -> see make_reconstruction method

@m-kowalska m-kowalska mentioned this issue Nov 19, 2018
ccluri pushed a commit that referenced this issue Nov 22, 2018
* tbasis with both cross-validation and l-curve on the same figure

* updated regularization method choice

* consistent documentation - solved issue #52.1

* changed name of the method recon to do_kcsd - solved issue #52.4

* refactoring of make_reconstruction method
@ccluri
Copy link
Collaborator Author

ccluri commented Nov 27, 2018

52.2 and 52.3 remain right? Or can we close this?
I mean, do you wish to stick to your original implementation, for what ever reason - then we can ignore these as issues.

@m-kowalska
Copy link
Collaborator

yes, 52.2 and 52.3 remain. I would like to refactor this part at same point.

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

No branches or pull requests

2 participants