Create info_functions.py #2

bisogni · 2018-07-09T21:29:05Z

"info_function.py" contains two functions, scan_info(num) and scan_dets(num), to retrieve easily information from a scan number 'num' within a notebook.

awalter-bnl

Congrats @bisogni you made it through to your first PR. Hopefully it wasn't to difficult. Overall I would like to see the style changes associated with our standard, this is best done using flake8 (see tutorial for instructions).

Also I would like to see a few tests added, particularly because this was broken during the recent ophyd upgrade and it would be good to catch these earlier.

awalter-bnl · 2018-07-10T10:38:49Z

sixtools/info_functions.py

+                    print('    {:_<30} : {:_>20}'.format(p, n))
+                print('{:-<70}'.format('-'))
+            except:
+                pass


I am not sure I like the idea of passing at this point. To me it means we are not providing all the information from the config attributes and this may get confusing. Can we just not format n in this part of the code?

@awalter-bnl I think that all the other information that prints out for our use case is not helpful. I can add an option for verbose = False to automatically pass on lists for now if you feel like it is important to let some access this information in an easy way, but I don't want to see it myself as it is not helpful.

Perhaps, in the future, easily recognized dictionary keys for this excess information can be configured so that it is easy to print or pass these things. See also bluesky/ophyd#587 and bluesky/ophyd#586

I don't feel really strongly on this to be honest. My conern is that the easiest way to describe what this does is it prints, in human readable form, the configuration attributes for the primary detectors. However if we are passing on the lists etc. then that is not strictily true and may lead to confusion. If you feel strongly enough then we can leave it as is (it is after all a six beamline function).

awalter-bnl · 2018-07-10T10:41:38Z

sixtools/info_functions.py

+
+    '''
+
+    if source in ['all','baseline','header']:


There are a few places where it doesn't follow the standard layout. For instance here there should be a space after each comma. @bisogni can I ask you to run this through flake8 and fix these errors, it would be a good learning oportunity? (I am very aware that this is stuff I wrote some time ago and it is wrong, ask @danielballan or @mrakitin about how often they have had to correct similar issues in my code !-) ).

awalter-bnl · 2018-07-11T10:14:20Z

I have thought about it more and am no longer asking for test functions. I think the test functions would need to work on recently created data from bluesky, as the most likely error is a change to the format of the metadata. As I am not convinced this is possible I think this test should be added to a series of tests that instead are run at the beamline just before/after an blueksy/ophyd update occurs. (perhaps the six team wants to start collecting this set of tests together?)

I would still like to see this tested with flake8 to remove any of the style errors.

ambarb · 2018-08-11T16:40:48Z

ok @awalter-bnl is this flak8 in the tutorial stuff that was forwarded for the development tutorial you gave use at 2ID? The webpage with the style guide and stuff??

mpmdean · 2018-08-15T16:17:58Z

Hi @ambarb

The flake8 tool is imposing a set of style rules called PEP8 available here:
https://www.python.org/dev/peps/pep-0008/

In general, I think it would make sense to be somewhat permissive of these sort of style issues with the provisos

The function(s) in questions are somewhat separate in the sense that we don't need to build other stuff on top of the functions
That we must develop sixtools towards a professional package that meets all these requirements. For example, we raise issues to provide these things in the future.

For the code in question, it's worth emphasizing that
header._repr_html_() is intended to be returning what the user wants to read. Obviously, it doesn't, but I think keeping a similar approach might be the way to go long term.

Since we're severely underresourced for developing the code, my own preference would be to add a warning to the existing code and then merging it would be the way to go. I wouldn't want to get too deep into perfecting the style issues before thinking about the methods, but I am also keen to have everyone participating and being able to share their code.

danielballan · 2018-08-20T19:25:18Z

I'll offer my two cents about flake8 and style:

For context, many scientific Python projects strive to meet this standard but few are strict about it. It's most valuable for medium-to-large projects, where it can be useful to let a "robot" police style so that humans can focus their energy on arguing about more substantive things.

I strongly encourage you to strive to keep to it, but if you decide to remove it from .travis.yml so that a style infraction does not cause the builds to "fail", that would not be unreasonable.

awalter-bnl · 2018-08-22T12:13:06Z

I second @danielballan on this, implementing the a 'common' style makes reading/understanding the code easier for non-authors and helps significantly when multiple authors are working on it. I would strongly suggest that this be kept.

To answer @ambarb question, the use of flake8 is described in the tutorial here http://nsls-ii.github.io/scientific-python-cookiecutter/the-code-itself.html#lint-check-for-suspicious-looking-code

mrakitin

@bisogni, thanks for the contribution. I am seconding @awalter-bnl's request to run it through flake8 and fix the styling issues first. Then I am happy to merge.

mrakitin · 2019-09-24T16:28:24Z

sixtools/info_functions.py

+
+    '''
+
+    if source in ['all','baseline','header']:


Suggested change

if source in ['all','baseline','header']:

if source in ['all', 'baseline', 'header']:

mrakitin · 2019-09-24T16:28:41Z

sixtools/info_functions.py

+
+
+###scan info utilities###
+def scan_info(scan_id,source='all'):


Suggested change

def scan_info(scan_id,source='all'):

def scan_info(scan_id, source='all'):

mrakitin · 2019-09-24T16:29:28Z

sixtools/info_functions.py

@@ -0,0 +1,94 @@
+
+
+###scan info utilities###


flake8 will complain about ###. Here is a fix:

Suggested change

###scan info utilities###

# scan info utilities

mrakitin · 2019-09-24T16:29:48Z

sixtools/info_functions.py

+    Parameters
+    ----------
+    scan_id : integer
+        The scan_id, or the location form latest scan_id using -1,-2......, to print data from


Suggested change

The scan_id, or the location form latest scan_id using -1,-2......, to print data from

The scan_id, or the location from latest scan_id using -1,-2......, to print data from

mrakitin · 2019-09-24T16:30:38Z

sixtools/info_functions.py

+    scan_id : integer
+        The scan_id, or the location form latest scan_id using -1,-2......, to print data from
+    source : string, optional
+        The source to display info from: can be 'all', 'header' (for header info) or 'baseline' (for baseline info). is 'All' by default.


Suggested change

The source to display info from: can be 'all', 'header' (for header info) or 'baseline' (for baseline info). is 'All' by default.

The source to display info from: can be 'all', 'header' (for header info) or 'baseline' (for baseline info). ``all`` is default.

Create info_functions.py

7377c6e

"info_function.py" contains two functions, scan_info(num) and scan_dets(num), to retrieve easily information from a scan number 'num' within a notebook.

bisogni added the enhancement New feature or request label Jul 9, 2018

bisogni assigned awalter-bnl Jul 9, 2018

bisogni requested a review from ambarb July 9, 2018 21:29

awalter-bnl reviewed Jul 10, 2018

View reviewed changes

ambarb mentioned this pull request Jul 10, 2018

extra - unwanted - information in config_attrs for qem (Quad-EM) bluesky/ophyd#587

Open

mrakitin requested changes Sep 24, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create info_functions.py #2

Create info_functions.py #2

bisogni commented Jul 9, 2018

awalter-bnl left a comment

awalter-bnl Jul 10, 2018

ambarb Jul 10, 2018

awalter-bnl Jul 10, 2018 •

edited

Loading

awalter-bnl Jul 10, 2018

awalter-bnl commented Jul 11, 2018

ambarb commented Aug 11, 2018

mpmdean commented Aug 15, 2018

danielballan commented Aug 20, 2018

awalter-bnl commented Aug 22, 2018

mrakitin left a comment

mrakitin Sep 24, 2019

mrakitin Sep 24, 2019

mrakitin Sep 24, 2019

mrakitin Sep 24, 2019

mrakitin Sep 24, 2019

	if source in ['all','baseline','header']:
	if source in ['all', 'baseline', 'header']:



		###scan info utilities###
		def scan_info(scan_id,source='all'):

	def scan_info(scan_id,source='all'):
	def scan_info(scan_id, source='all'):

	The scan_id, or the location form latest scan_id using -1,-2......, to print data from
	The scan_id, or the location from latest scan_id using -1,-2......, to print data from

	The source to display info from: can be 'all', 'header' (for header info) or 'baseline' (for baseline info). is 'All' by default.
	The source to display info from: can be 'all', 'header' (for header info) or 'baseline' (for baseline info). ``all`` is default.

Create info_functions.py #2

Are you sure you want to change the base?

Create info_functions.py #2

Conversation

bisogni commented Jul 9, 2018

awalter-bnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awalter-bnl Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awalter-bnl commented Jul 11, 2018

ambarb commented Aug 11, 2018

mpmdean commented Aug 15, 2018

danielballan commented Aug 20, 2018

awalter-bnl commented Aug 22, 2018

mrakitin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awalter-bnl Jul 10, 2018 •

edited

Loading