-
Notifications
You must be signed in to change notification settings - Fork 0
Prepare for v0.0.9 release #14
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
Conversation
* Undo/redo is broken for manual editing * Sample editing is still not fully implemented
Thank you for your contribution to easyInterface.
|
Codecov Report
@@ Coverage Diff @@
## pre-release #14 +/- ##
============================================
Coverage 85.42% 85.42%
============================================
Files 18 18
Lines 2944 2944
============================================
Hits 2515 2515
Misses 429 429 Continue to review full report at Codecov.
|
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 just have a few minor comment, as added above, to be considered for future code refactoring.
@@ -48,6 +54,20 @@ | |||
} | |||
} | |||
|
|||
POLAR_DETAILS = { |
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 would propose to use a full name here, POLARIZATION_DETAILS, because POLAR sounds more like polar coordinates.
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.
Modified
@@ -254,16 +282,20 @@ def default(cls, polarised: bool = False): | |||
x = [] | |||
y_obs = [] | |||
sy_obs = [] | |||
y_obs_diff = None | |||
sy_obs_diff = None | |||
y_obs_up = None | |||
sy_obs_up = None | |||
y_obs_down = None | |||
sy_obs_down = None | |||
if polarised: |
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.
We should consider to make a enum with type of experiment, instead of using this boolean polarised
.
@@ -324,11 +358,66 @@ def __repr__(self) -> str: | |||
return '{} Experimental phases'.format(len(self)) | |||
|
|||
|
|||
class chi2(LoggedPathDict): |
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.
We should consider to rename this class into something like RefinementType, as chi2 is associated with calculation of the goodness-of-fit.
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.
Modified
@@ -353,6 +446,10 @@ def __init__(self, name: str, wavelength: Base, offset: Base, phase: ExperimentP | |||
self.setItemByPath(['offset', 'tooltip'], EXPERIMENT_DETAILS['offset']['tooltip']) | |||
self.setItemByPath(['offset', 'url'], EXPERIMENT_DETAILS['offset']['url']) | |||
|
|||
self.setItemByPath(['field', 'header'], EXPERIMENT_DETAILS['field']['header']) |
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.
field
should probably be renamed into magnetic_field
, because electric field could also be a part of metadata.
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.
Modified
@@ -352,7 +368,7 @@ def updatePhases(self) -> NoReturn: | |||
self.project_dict.setItemByPath(['info', 'phase_ids'], list(phases.keys())) | |||
self.project_dict.endBulkUpdate() | |||
else: | |||
k, v = self.project_dict['phases'].dictComparison(phases) | |||
k, v, t = self.project_dict['phases'].dictComparison(phases) |
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.
Should we use a less cryptic names, such as keys
, values
? And what is t
? It seems that t
is not used.
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.
Modified
@@ -473,7 +507,7 @@ def setPhase(self, phase: Phase) -> NoReturn: | |||
if isinstance(phase, Phase): | |||
new_phase_name = phase['phasename'] | |||
if new_phase_name in self.project_dict['phases'].keys(): | |||
k, v = self.project_dict.getItemByPath(['phases', new_phase_name]).dictComparison(phase) | |||
k, v, t = self.project_dict.getItemByPath(['phases', new_phase_name]).dictComparison(phase) |
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.
Should we use a less cryptic names, such as keys
, values
? And what is t
? It seems that t
is not used.
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.
Modified
@@ -492,7 +526,7 @@ def setPhases(self, phases: Union[Phase, Phases]) -> NoReturn: | |||
""" | |||
if isinstance(phases, Phase): | |||
new_phase_name = phases['phasename'] | |||
k, v = self.project_dict.getItemByPath(['phases', new_phase_name]).dictComparison(phases) | |||
k, v, t = self.project_dict.getItemByPath(['phases', new_phase_name]).dictComparison(phases) |
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.
Should we use a less cryptic names, such as keys
, values
? And what is t
? It seems that t
is not used.
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.
Modified
@@ -547,7 +581,7 @@ def setExperiment(self, experiment: Experiment) -> NoReturn: | |||
if isinstance(experiment, Experiment): | |||
new_phase_name = experiment['name'] | |||
if new_phase_name in self.project_dict['experiments'].keys(): | |||
k, v = self.project_dict.getItemByPath(['experiments', new_phase_name]).dictComparison(experiment) | |||
k, v, t = self.project_dict.getItemByPath(['experiments', new_phase_name]).dictComparison(experiment) |
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.
Should we use a less cryptic names, such as keys
, values
? And what is t
? It seems that t
is not used.
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.
Modified
except TypeError as ex: | ||
raise KeyError(str(ex)) | ||
|
||
def _realSetItemByPath(self, keys: list, value: Any) -> NoReturn: | ||
"""Actually sets the value in a nested object by the key sequence.""" | ||
self.getItemByPath(keys[:-1])[keys[-1]] = value | ||
|
||
def _realAddItemByPath(self, keys: list, value: Any) -> NoReturn: | ||
"""Actually sets the value in a nested object by the key sequence.""" |
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.
This comment seems to be not updated.
@@ -313,7 +321,7 @@ def asDict(self) -> dict: | |||
base_dict[key] = item.asDict() | |||
return base_dict | |||
|
|||
def dictComparison(self, another_dict: Union['PathDict', dict], ignore=None) -> Tuple[list, list]: | |||
def dictComparison(self, another_dict: Union['PathDict', dict], ignore=None) -> Tuple[list, list, list]: | |||
""" | |||
Compare self to a dictionary or PathDict and return the update path and value |
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.
This comment seems to be not updated.
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.
This part looks okay.
* Prepare for v0.0.9 release (#14) * Add polarised experiment * Add chi2 options * Calculate profile up/down * RE-implement chi2 * Add Polarization efficiency/level * Fix fitable values * Fix hidden refinement * Remove auto-refine * Temporary fix of min/max for polarization and efficiency * Fix calculated pattern values * Add calculated background * Fix background for polarised neutrons * Fixup creation of phaess and exps * Typo fixes [ci skip] * Fix polarisation (until cryspy is fixed) * Fix refine polarisation * Fix error on scale * Don't print to the console * Create empty cryspy obj if rcif path is None * Fix experiments as cif * 1st implementation of editable CIF at the back end * Undo/redo is broken for manual editing * Sample editing is still not fully implemented * Fix issue with complex numbers for the scattering length * Change space group name source * Add exp from cif compatibility * Add phase from cif compatibility * Allow to run bulk update without macro * Add datablock label to calculations cif * Part of back-end implementation of Main.cif Edit * Remove global_ from CIFs on saving * Simplify writing CIF files * Save calculations.cif to project * Switch to master branch for cryspy * Add calculations.cif * Intermediate fixes * Re-write of DictComparison * Fix unit tests and dictTools * fix fit tests on windows/linux * More windows test fixes * Fix undo/redo for list types * More undo/redo work * Sort Backgrounds * Ugly hack to save name/keyword changes * Change keyword list to string * Sort background list of string keys numerically * Move filenames to master obj * Internal main_rcif to project_rcif * Move from phases to samples * Variable name changes * Rename phases to sample Co-authored-by: Andrew Sazonov <[email protected]> * Update Release.json Co-authored-by: Andrew Sazonov <[email protected]>
Update to easyInterface including polarization and dictionary fixes
Before submitting this PR, please make sure: