-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add new PyChop version and create model versioning #16
base: main
Are you sure you want to change the base?
Conversation
The model properties will not work yet
Can we xfail the appropriate tests so we get a green tick and verify nothing else was broken? |
I've changed the tests so that the old tests test the |
longest = np.array(longest) + padding | ||
out = [] | ||
separator = '|' + '|'.join(['-' * (i + 1) for i in longest]) + '|' | ||
|
||
for i, line in enumerate(contents): | ||
new_line = '| ' | ||
for val, length in zip(line, longest): | ||
if val is None: | ||
new_line += ' ' * length + '| ' | ||
else: | ||
new_line += val + ' ' * (length - len(val)) + '| ' | ||
|
||
if line[0] is not None: | ||
if i == 1: | ||
out.append(separator.replace('-', '=')) | ||
else: | ||
out.append(separator) | ||
|
||
out.append(new_line) | ||
|
||
out.append(separator) | ||
|
||
return '\n'.join(out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's just my functional-programming brain, but you can avoid a lot of this fuss by using Python's format functions. It needs some hackery to get the same table, but a similar table could be done without incrementing padding
and preproc
, different formats, etc. sep_nl
is just because you can't have \
in an f-string.
longest = np.array(longest) + padding | |
out = [] | |
separator = '|' + '|'.join(['-' * (i + 1) for i in longest]) + '|' | |
for i, line in enumerate(contents): | |
new_line = '| ' | |
for val, length in zip(line, longest): | |
if val is None: | |
new_line += ' ' * length + '| ' | |
else: | |
new_line += val + ' ' * (length - len(val)) + '| ' | |
if line[0] is not None: | |
if i == 1: | |
out.append(separator.replace('-', '=')) | |
else: | |
out.append(separator) | |
out.append(new_line) | |
out.append(separator) | |
return '\n'.join(out) | |
padding = padding + 1 | |
preproc = [["-" if elem is None else elem for elem in row] for row in contents] | |
sep_line = f"|{'|'.join('-' * (i+padding) for i in longest)}|" | |
header_sep_line = sep_line.replace("-", "=") | |
sep_nl = "\n" + sep_line | |
data_line = "|" + "|".join(f"{{:^{length+padding}}}" for length in longest) + "|" | |
formatted = map(lambda x: data_line.format(*x), preproc) | |
out = f"""\ | |
{sep_line} | |
{next(formatted)} | |
{header_sep_line} | |
{sep_nl.join(formatted)} | |
{sep_line} | |
""" | |
return out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not quite equivalent (though I am open to discuss the best way to format this nicely) - I expect it will be possible to achieve similar formatting (if desired) using this new method as well, but I am unsure I will be able to get there easily by myself.
Old implementation (using a fake example):
|------------------|------------------|--------------------|
| MODEL | ALIAS FOR | CONFIGURATIONS |
|==================|==================|====================|
| PyChop_fit | PyChop_fit_v2 | |
|------------------|------------------|--------------------|
| PyChop_fit_v1 | | chopper_package |
|------------------|------------------|--------------------|
| PyChop_fit_v2 | | chopper_package |
|------------------|------------------|--------------------|
| test_model | test_model_v4 | |
|------------------|------------------|--------------------|
| test_model_v1 | | config1 |
| | | config2 |
|------------------|------------------|--------------------|
| test_model_v2 | | config1 |
| | | config2 |
|------------------|------------------|--------------------|
| test_model_v3 | | config1 |
| | | config2 |
|------------------|------------------|--------------------|
| test_model_v4 | | config1 |
| | | config2 |
| | | config3 |
| | | config4 |
|------------------|------------------|--------------------|
and
|------------------|------------------|--------------------|-------------------------------|
| MODEL | ALIAS FOR | CONFIGURATIONS | OPTIONS |
|==================|==================|====================|===============================|
| PyChop_fit | PyChop_fit_v2 | | |
|------------------|------------------|--------------------|-------------------------------|
| PyChop_fit_v1 | | chopper_package | SEQ-100-2.0-AST |
| | | | SEQ-700-3.5-AST |
| | | | ARCS-100-1.5-AST (default) |
| | | | ARCS-700-1.5-AST |
| | | | ARCS-700-0.5-AST |
| | | | ARCS-100-1.5-SMI |
| | | | ARCS-700-1.5-SMI |
|------------------|------------------|--------------------|-------------------------------|
| PyChop_fit_v2 | | chopper_package | SEQ-100-2.0-AST |
| | | | SEQ-700-3.5-AST |
| | | | ARCS-100-1.5-AST (default) |
| | | | ARCS-700-1.5-AST |
| | | | ARCS-700-0.5-AST |
| | | | ARCS-100-1.5-SMI |
| | | | ARCS-700-1.5-SMI |
|------------------|------------------|--------------------|-------------------------------|
| test_model | test_model_v4 | | |
|------------------|------------------|--------------------|-------------------------------|
| test_model_v1 | | config1 | A (default) |
| | | | N |
| | | | |
| | | config2 | B (default) |
| | | | C |
| | | | D |
|------------------|------------------|--------------------|-------------------------------|
| test_model_v2 | | config1 | A (default) |
| | | | N |
| | | | |
| | | config2 | B (default) |
| | | | C |
| | | | D |
|------------------|------------------|--------------------|-------------------------------|
| test_model_v3 | | config1 | A (default) |
| | | | N |
| | | | |
| | | config2 | B (default) |
| | | | C |
| | | | D |
| | | | E |
|------------------|------------------|--------------------|-------------------------------|
| test_model_v4 | | config1 | A (default) |
| | | | N |
| | | | |
| | | config2 | B (default) |
| | | | C |
| | | | D |
| | | | |
| | | config3 | X (default) |
| | | | Y |
| | | | |
| | | config4 | W (default) |
| | | | R |
|------------------|------------------|--------------------|-------------------------------|
This implementation:
|------------------|------------------|--------------------|
| MODEL | ALIAS FOR | CONFIGURATIONS |
|==================|==================|====================|
| PyChop_fit | PyChop_fit_v2 | - |
|------------------|------------------|--------------------|
| PyChop_fit_v1 | - | chopper_package |
|------------------|------------------|--------------------|
| PyChop_fit_v2 | - | chopper_package |
|------------------|------------------|--------------------|
| test_model | test_model_v4 | - |
|------------------|------------------|--------------------|
| test_model_v1 | - | config1 |
|------------------|------------------|--------------------|
| - | - | config2 |
|------------------|------------------|--------------------|
| test_model_v2 | - | config1 |
|------------------|------------------|--------------------|
| - | - | config2 |
|------------------|------------------|--------------------|
| test_model_v3 | - | config1 |
|------------------|------------------|--------------------|
| - | - | config2 |
|------------------|------------------|--------------------|
| test_model_v4 | - | config1 |
|------------------|------------------|--------------------|
| - | - | config2 |
|------------------|------------------|--------------------|
| - | - | config3 |
|------------------|------------------|--------------------|
| - | - | config4 |
|------------------|------------------|--------------------|
and
|------------------|------------------|--------------------|-------------------------------|
| MODEL | ALIAS FOR | CONFIGURATIONS | OPTIONS |
|==================|==================|====================|===============================|
| PyChop_fit | PyChop_fit_v2 | - | - |
|------------------|------------------|--------------------|-------------------------------|
| PyChop_fit_v1 | - | chopper_package | SEQ-100-2.0-AST |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | SEQ-700-3.5-AST |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | ARCS-100-1.5-AST (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | ARCS-700-1.5-AST |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | ARCS-700-0.5-AST |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | ARCS-100-1.5-SMI |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | ARCS-700-1.5-SMI |
|------------------|------------------|--------------------|-------------------------------|
| PyChop_fit_v2 | - | chopper_package | SEQ-100-2.0-AST |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | SEQ-700-3.5-AST |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | ARCS-100-1.5-AST (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | ARCS-700-1.5-AST |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | ARCS-700-0.5-AST |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | ARCS-100-1.5-SMI |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | ARCS-700-1.5-SMI |
|------------------|------------------|--------------------|-------------------------------|
| test_model | test_model_v4 | - | - |
|------------------|------------------|--------------------|-------------------------------|
| test_model_v1 | - | config1 | A (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | N |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | - |
|------------------|------------------|--------------------|-------------------------------|
| - | - | config2 | B (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | C |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | D |
|------------------|------------------|--------------------|-------------------------------|
| test_model_v2 | - | config1 | A (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | N |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | - |
|------------------|------------------|--------------------|-------------------------------|
| - | - | config2 | B (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | C |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | D |
|------------------|------------------|--------------------|-------------------------------|
| test_model_v3 | - | config1 | A (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | N |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | - |
|------------------|------------------|--------------------|-------------------------------|
| - | - | config2 | B (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | C |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | D |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | E |
|------------------|------------------|--------------------|-------------------------------|
| test_model_v4 | - | config1 | A (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | N |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | - |
|------------------|------------------|--------------------|-------------------------------|
| - | - | config2 | B (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | C |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | D |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | - |
|------------------|------------------|--------------------|-------------------------------|
| - | - | config3 | X (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | Y |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | - |
|------------------|------------------|--------------------|-------------------------------|
| - | - | config4 | W (default) |
|------------------|------------------|--------------------|-------------------------------|
| - | - | - | R |
|------------------|------------------|--------------------|-------------------------------|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just because I stuck in centred (^
) rather than left aligned (<
) in the formats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change data_line
to:
data_line = "| " + "| ".join(f"{{:<{length+padding}}}" for length in longest) + "|"
And it should do what you had before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I see what you mean, it was grouping things previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, do you want to go ahead with this? With both split_on_model
and _format_table
in a new utils module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are they likely to be re-used? If so then collecting some generic output-printing utils makes sense. This kind of modularity is good for testing.
But if they would have private names and be used in one place, I'm not sure it's so beneficial to split them into a new module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at the moment, no. In the future, difficult to say, but I can't think of anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're not sure, the safest option is private functions in the existing module. As soon as we want to re-use them, they can "go public" in a logical place. But the reverse maneuver is an API-breaking change.
(Ok, we're allowed those at this moment, but I suspect the timescale for "added another feature that prints tables" could overlap with the stable release schedule.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give me an example of how the split_on_model
is supposed to be used inside _format_table
? Can't seem to figure out how it's supposed to replace more_itertools.split_before
for model_name, model_data in self._models.items(): | ||
length = len(model_name) | ||
if length > longest_name: | ||
longest_name = length | ||
|
||
if isinstance(model_data, str): | ||
contents.append([model_name, model_data, None]) | ||
|
||
length = len(model_data) | ||
if length > longest_alias: | ||
longest_alias = length | ||
else: | ||
for i, config_name in enumerate(model_data['configurations']): | ||
if i == 0: | ||
contents.append([model_name, None, config_name]) | ||
else: | ||
contents.append([None, None, config_name]) | ||
|
||
length = len(config_name) | ||
if length > longest_config: | ||
longest_config = length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it requires 2 runs through the elements, I would honestly not compute the length in this building-the-list stage as it makes the loop a lot more complex.
I would iterate through the compiled list and compute the longest in each column after this. The list will be short (<1k elements), so the cost of multiple runs (unless this function is called a lot) is irrelevant.
Pulling the logic out and using:
for name, alias, config in contents:
longest_name = max(longest_name, len(name))
longest_alias = max(longest_alias, len(alias))
longest_config = max(longest_config, len(config))
(or something more efficient) at the end makes the purpose clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe inside _format_table
, we could do (though we would either need to do masking or change the None
into e.g. "-"
):
contents = np.array(contents)
longest = np.max(np.vectorize(len)(b), axis=0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems promising. It should be done after the None->str conversion, just in case we decide to replace None with "----------"
😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better to do that upstream, in format_available_models_and_configurations
and format_available_models_options
, and completely eliminite the None
s as well as the pre-processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With those sort of details already taken care of, it becomes more practical to have a generic table formatter that e.g. supports multiple criteria of how blocks are split. (Or maybe just requires the blocks to be pre-split as well.)
The previous changes were wrong - they broke the method
try: | ||
model = self._models[model_name] | ||
except KeyError: | ||
raise InvalidModelError(model_name, self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be appropriate for these to be chained exceptions? https://peps.python.org/pep-3134/
except KeyError as e:
raise InvalidModelError(model_name, self) from e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure the additional information would be useful - the main purpose of this (and similar) exceptions is to communicate to the user that their input was wrong (and how to correct it), knowing that a KeyError
was involved seems like unnecessary information. I don't think there would be any harm in the chaining, per se, so I am not necessarily opposed, but I also don't see the reason for - do you have any particular reason for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can give a cleaner output than "while handling X, Y was raised", but I'm not super familiar with it. Try raising both and see which output looks better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WITHOUT from
:
Traceback (most recent call last):
File "C:\Users\dni83241\Documents\Git\resolution_functions\src\resolution_functions\instrument.py", line 466, in _resolve_model
model = self._models[model_name]
KeyError: 'PyChop_fit4'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "C:\Users\dni83241\AppData\Local\JetBrains\PyCharm Community Edition 2024.2.1\plugins\python-ce\helpers\pydev\pydevconsole.py", line 364, in runcode
coro = func()
File "<input>", line 1, in <module>
File "C:\Users\dni83241\Documents\Git\resolution_functions\src\resolution_functions\instrument.py", line 468, in _resolve_model
raise InvalidModelError(model_name, self)
resolution_functions.instrument.InvalidModelError: "PyChop_fit4" is not a valid model for the MAPS instrument version MAPS. This instrument only supports the following models: ['PyChop_fit'].
WITH from
:
Traceback (most recent call last):
File "C:\Users\dni83241\Documents\Git\resolution_functions\src\resolution_functions\instrument.py", line 466, in _resolve_model
model = self._models[model_name]
KeyError: 'PyChop_fit4'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "C:\Users\dni83241\AppData\Local\JetBrains\PyCharm Community Edition 2024.2.1\plugins\python-ce\helpers\pydev\pydevconsole.py", line 364, in runcode
coro = func()
File "<input>", line 1, in <module>
File "C:\Users\dni83241\Documents\Git\resolution_functions\src\resolution_functions\instrument.py", line 468, in _resolve_model
raise InvalidModelError(model_name, self) from e
resolution_functions.instrument.InvalidModelError: "PyChop_fit4" is not a valid model for the MAPS instrument version MAPS. This instrument only supports the following models: ['PyChop_fit'].
Now that you mention it, they are (almost) equally bad - I would prefer the KeyError
not to show up at all (wasn't that the default behaviour?) - but the change in text in from e
syntax makes it clearer that it is an intended behaviour, so we can go with that at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more subtle than I expected! Chaining looks slightly more correct, but not a big deal really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the output I was hoping for can be obtained via raise InvalidModelError() from None
, which gives:
Traceback (most recent call last):
File "C:\Users\dni83241\AppData\Local\JetBrains\PyCharm Community Edition 2024.2.1\plugins\python-ce\helpers\pydev\pydevconsole.py", line 364, in runcode
coro = func()
File "<input>", line 1, in <module>
File "C:\Users\dni83241\Documents\Git\resolution_functions\src\resolution_functions\instrument.py", line 468, in _resolve_model
raise InvalidModelError(model_name, self) from None
resolution_functions.instrument.InvalidModelError: "invalid" is not a valid model for the ARCS instrument version ARCS. This instrument only supports the following models: ['PyChop_fit'].
any thoughts?
""" | ||
A list of all :term:`models<model>` available for this :term:`version` of this | ||
:term:`instrument`. | ||
|
||
Includes both all the versions of all models and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and ...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now updated
longest = np.array(longest) + padding | ||
out = [] | ||
separator = '|' + '|'.join(['-' * (i + 1) for i in longest]) + '|' | ||
|
||
for i, line in enumerate(contents): | ||
new_line = '| ' | ||
for val, length in zip(line, longest): | ||
if val is None: | ||
new_line += ' ' * length + '| ' | ||
else: | ||
new_line += val + ' ' * (length - len(val)) + '| ' | ||
|
||
if line[0] is not None: | ||
if i == 1: | ||
out.append(separator.replace('-', '=')) | ||
else: | ||
out.append(separator) | ||
|
||
out.append(new_line) | ||
|
||
out.append(separator) | ||
|
||
return '\n'.join(out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are they likely to be re-used? If so then collecting some generic output-printing utils makes sense. This kind of modularity is good for testing.
But if they would have private names and be used in one place, I'm not sure it's so beneficial to split them into a new module.
try: | ||
model = self._models[model_name] | ||
except KeyError: | ||
raise InvalidModelError(model_name, self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can give a cleaner output than "while handling X, Y was raised", but I'm not super familiar with it. Try raising both and see which output looks better?
for model_name, model_data in self._models.items(): | ||
length = len(model_name) | ||
if length > longest_name: | ||
longest_name = length | ||
|
||
if isinstance(model_data, str): | ||
contents.append([model_name, model_data, None]) | ||
|
||
length = len(model_data) | ||
if length > longest_alias: | ||
longest_alias = length | ||
else: | ||
for i, config_name in enumerate(model_data['configurations']): | ||
if i == 0: | ||
contents.append([model_name, None, config_name]) | ||
else: | ||
contents.append([None, None, config_name]) | ||
|
||
length = len(config_name) | ||
if length > longest_config: | ||
longest_config = length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems promising. It should be done after the None->str conversion, just in case we decide to replace None with "----------"
😉
configurations: &configurations | ||
chopper_package: | ||
configurations: | ||
chopper_package: &configurations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning to move this rather than rename it as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
laziness, lack of attention to detail ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the human condition"
The get_model_signature method needs the actual name of the model - which _resolve_model was not returning (only the dict). _resolve_model now returns the new model_name as well. This should have the added benefit of making the logging of the new name easier in the future.
Resolves the PyChop issue documented at Mantid#38591
Simultaneously, applies new versioning scheme for models, where all models are versioned with
_v{int}
at the end of their names to keep record of model changes, and aliases are created without the version to provide seamless access to the latest version of models.Tests will be failing until the changes are also applied to https://github.com/mducle/pychop