BlueBrain / NeuroTS

Topological Neuron Synthesis
https://neurots.readthedocs.io/en/stable
Apache License 2.0
36 stars 5 forks source link

Feat: Rework input validation process #52

Closed adrien-berchet closed 1 year ago

codecov[bot] commented 2 years ago

Codecov Report

Merging #52 (500c539) into main (c43fa25) will increase coverage by 0.06%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   97.36%   97.43%   +0.06%     
==========================================
  Files          33       38       +5     
  Lines        1900     1951      +51     
  Branches      281      288       +7     
==========================================
+ Hits         1850     1901      +51     
  Misses         34       34              
  Partials       16       16              
Flag Coverage Δ
pytest 97.43% <100.00%> (+0.06%) :arrow_up:

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

Impacted Files Coverage Δ
neurots/generate/algorithms/tmdgrower.py 99.25% <100.00%> (-0.03%) :arrow_down:
neurots/preprocess/__init__.py 100.00% <100.00%> (ø)
neurots/preprocess/exceptions.py 100.00% <100.00%> (ø)
neurots/preprocess/relevance_checkers.py 100.00% <100.00%> (ø)
neurots/preprocess/utils.py 100.00% <100.00%> (ø)
neurots/preprocess/validity_checkers.py 100.00% <100.00%> (ø)
eleftherioszisis commented 2 years ago

+1 for the design.

adrien-berchet commented 2 years ago

I reorganized the code so that all the check/preprocess functions are in the preprocess module. I created 3 kind of functions:

WDYT @eleftherioszisis @lidakanari ?

arnaudon commented 2 years ago

LGTM, feel free to merge and update mine if it needs to be adapted to this

eleftherioszisis commented 2 years ago

I have a few minor comments, but overall it's legitimate. One last thing, can we please use "relevance" instead of "relevancy"? I know they are synonyms, but my brain is triggered because "relevance" is the most used word. :D

adrien-berchet commented 2 years ago

I have a few minor comments, but overall it's legitimate. One last thing, can we please use "relevance" instead of "relevancy"? I know they are synonyms, but my brain is triggered because "relevance" is the most used word. :D

I changed for relevance, I didn't know it was more used than relevancy, I guess I'm an old school guy :)

eleftherioszisis commented 2 years ago

I am good with the changes. @lidakanari I leave it to you to approve.

arnaudon commented 1 year ago

I just rebased on main, I guess if CI pass we can merge?