find_sample_from_2dscan#57
find_sample_from_2dscan#57MehmetTopsakal wants to merge 4 commits intoxpdAcq:masterfrom MehmetTopsakal:master
Conversation
add find_sample_from_2dscan
CJ-Wright
left a comment
CJ-Wright
left a comment
There was a problem hiding this comment.
This looks great!
I have a few inline comments.
Would you be interested in writing a test? (If not I can push up tests to this branch).
xpdtools/tools.py
Outdated
|
|
||
| if params is None: | ||
| print('params is not provided. Using default parameters') | ||
| params = {'eps':0.05,'min_samples':20,'n_jobs':1, |
There was a problem hiding this comment.
Would it be possible to have these as keyword arguments to the function?
For example:
def find_sample_from_2dscan(I_arr, xy_arr, Q_arr=None, eps=0.05, min_samples=20, n_jobs=1, ...):There was a problem hiding this comment.
@CJ-Wright How do we do the testing?
The default parameters are working for 44 different samples.
See this gif:
xpdtools/tools.py
Outdated
|
|
||
| """ | ||
|
|
||
| from sklearn.cluster import DBSCAN |
There was a problem hiding this comment.
Would it be possible to put these imports at the top of the module?
| 's_ratio':0.5,'qrange':(1,5), | ||
| 'use_unclassified':True} | ||
|
|
||
| if isinstance(Q_arr,np.ndarray): |
There was a problem hiding this comment.
I think this might be able to be if Q_arr since None is False like
Codecov Report
@@ Coverage Diff @@
## master #57 +/- ##
==========================================
- Coverage 87.43% 84.88% -2.55%
==========================================
Files 16 16
Lines 748 774 +26
==========================================
+ Hits 654 657 +3
- Misses 94 117 +23
Continue to review full report at Codecov.
|
|
This seems to be failing due to anaconda.org 503 errors so we might give it a few mins and try the build again. |
|
How many points could we get away with for the tests? |
sbillinge
left a comment
sbillinge
left a comment
There was a problem hiding this comment.
please see my inline comment. I think we need a conversation to decide what goes where.
Also, make sure that @MehmetTopsakal is up to speed with testing.
|
|
||
|
|
||
|
|
||
| def find_sample_from_2dscan(I_arr, xy_arr, Q_arr=None, |
There was a problem hiding this comment.
I am a little confused about whether this is in the right place. It seems that most things in xpdtools.tools are fairly low-level functions for the functioning of the pipelines. On the other hand, I think this is some kind of script for actually doing xy scans to find a sample? Or is it for analyzing 2d data from such a scan to find positions of the sample given the data? I am afraid that if we don't hae the right structure to our files and projects it will become a maintenance (and user) nightmare. Can we have a discussion (maybe a call) to discuss some of these issues? I would like to define different activities and group code accordingly.
4 participants
find_sample_from_2dscan