Fix 78

[zakv]

This PR attempts to resolve #78

Changes:

• Prevent Run.set_group() from creating the group in the hdf5 file if self.no_write is True.
• Make Run._create_group_if_not_exists() raise an error if the group doesn't already exist but self.no_write is True.
• Run._create_group_if_not_exists() now catches the ValueError raised if something else creates the group between when Run._create_group_if_not_exists() sees that the group doesn't exist and when it actually tries to create the group.
• Ditch the usage of self._no_group to backup/restore self.no_write.
  • Instead set self.group to None by default if there's no lyse script running to name it after, then set the save methods to raise an exception if they need self.group but it is None.
• Removed the print() statement from Run.save_results().
• Make Run's h5_path, no_write, and group attributes private properties.
• Make error messages in Run.save_result() and Run.save_result_array() more accurate.
• Resolve the bug where Sequence.__init__() would fail if the hdf5 file didn't already exist, first brought up in PR Bugfix for sequence #73 .
• Refactor Sequence.__init__() to use Run.__init__().
• Add some docstrings in __init__.py.
• Fix a few typos in other parts of the code.

[zakv]

A few other points that I was hoping for feedback on before making any changes:

Is the print statement here:

lyse/lyse/__init__.py

Lines 277 to 286 in 0910447

	def save_results(self, *args, **kwargs):
	"""Iteratively call save_result() on multiple results.
	Assumes arguments are ordered such that each result to be saved is
	preceeded by the name of the attribute to save it under.
	Keywords arguments are passed to each call of save_result()."""
	names = args[::2]
	values = args[1::2]
	for name, value in zip(names, values):
	print('saving %s =' % name, value)
	self.save_result(name, value, **kwargs)

intended to be there because it's somewhat easy to accidentally call the method incorrectly? Or was that a debugging print statement that got left in accidentally?

Also, as currently written, the group argument for the save...() methods doesn't get '/results/' prepended to it automatically. That means that to save to e.g. '/results/results_group' users either have to call run.set_group('results_group') or run.save_result('..., group='/results/results_group'). It would be easy to forget to manually prepend '/results/' in the latter. Should I adjust the saving methods to automatically prepend '/results/' to the group argument? I suppose that would technically break backwards compatibility, though I'm not sure if many people use that argument.

[philipstarkey]

So the print statement has been there for over 9 years (since before it was even named lyse - which is as far back as I can check). I would nuke it provided you add a docstring to that method (so that users have a way to not be confused!)

I think the lack or '/results/' prepend is so that you can save results anywhere in the file if you want. This was probably introduced for Monash labs who wanted to overwrite the images saved by BIAS? Either way I think it has to stay as is. You should be calling set_group() if you are running from a terminal (running from a file negates the need for an explicit call to set_group()). The group argument of the save* methods should only be used if you want to save to an arbitrary location (and future docstrings should be written to reflect that).

[philipstarkey]

Couple of other suggested changes:

1. This one is technically backwards incompatible, but I think we should just do it anyway. I would rename self.no_write to self.__no_write so that it can't be changed after instantiation. This variable should never have been public, so I would treat the rename as a bug fix so we don't have to bump the major version. To be honest..., h5_path and group should be private too. Gah, this is a bit of scope creep, but do you want to rename those variables to self.__... and replace all 3 of them with read only Python properties with the original names so that backwards compatibility is partially maintained? Reading the values is fine, being able to change them is a bug.

2. There are some exceptions raised in save_result_array and save_result which sort of imply that no_write will only be set when using a Sequence object. This isn't really true - Run objects could be deliberately created in read only mode by the user.

3. The first line of the Run docstring uses the word "run" when you mean "shot". This is of course because the class name is wrong (historical holdover from before we had settled on shot). At the very least we should make the docstring use "shot" instead of "run". That would also necessitate updating a few of the error messages to use the word shot instead of run. I'm going to log a bug for renaming and deprecating Run while I'm at it.

4. Does this interact with PR Bugfix for sequence #73? It was a minimal change proposed there...do you want to integrate it into yours (see my comments on that PR for changes) and update to be consistent with the current no_write changes?

Think that's everything from me! Sorry it's a bit of scope creep.

[zakv]

Ok! I believe that last batch of commits resolves all of the issues you've mentioned, including the bug that PR #73 aims to fix.

I think the lack or '/results/' prepend is so that you can save results anywhere in the file if you want.

Ah, I had assumed that using '..' would let you easily access the root directory from /results but seems I was wrong. It definitely makes sense to leave it as is.

[philipstarkey]

Looking good! Couple more review comments for your latest changes.

[zakv]

Also, I just noticed issue #37. This PR sort of negates that issue since it removes that print statement.

[philipstarkey]

Cool! I will merge this.

I've also logged #83 which is sort of related, but too hard to fix here since it is historic behaviour. I'll probably close #37 in favour of #84 as well.