fix optimization flag in ParameterHelper class #191
Conversation
Solve unit tests failing due to pytest
…ment Conflicts: requirements.txt
| self.logger.debug(f" using hyper-parameters of {sig:6.2g} " | ||
| f"{ls:6.2g}") | ||
| f"{ls:6.2g} {opt_sig} {opt_ls}") | ||
| self.hyps_opt[group_type] = all_opt_sig + all_opt_ls |
There was a problem hiding this comment.
So to confirm, it's okay that it's all signal hyps and then all length scale params within the hyps_opt indexes? If so, this looks good to me.
There was a problem hiding this comment.
that's exactly where the bug was at. The hyper-parameter array is always ordered like "sig2-1 sig2-2 ls2-1 ls2-2 sig3-1 sig3-2 ..."
|
At the risk of making a trite critique: Is it possible to unit test this logic (that constraints on optimization operate in the right way) slash were the previous unit tests not robust enough to catch this? |
| species_labels = [] | ||
| coded_species = [] | ||
| for i, ele in enumerate(unique_species): | ||
| if isinstance(ele, str): | ||
| self.species_labels.append(ele) | ||
| self.coded_species.append(element_to_Z(ele)) | ||
| species_labels.append(ele) | ||
| coded_species.append(element_to_Z(ele)) | ||
| elif isinstance(ele, int): | ||
| self.coded_species.append(ele) | ||
| self.species_labels.append(Z_to_element(ele)) | ||
| coded_species.append(ele) | ||
| species_labels.append(Z_to_element(ele)) | ||
| else: | ||
| print("element type not accepted", ele, type(ele)) | ||
| sort_id = np.argsort(coded_species) | ||
| for i in sort_id: | ||
| self.coded_species.append(coded_species[i]) | ||
| self.species_labels.append(species_labels[i]) |
There was a problem hiding this comment.
A quick question: Is there anything that we could do here to make this future-proofed for coarse graining? Maybe it's thinking too far ahead, but just so that if a user were to pass in a number outside of {1...118} or a species label outside of the periodic table that everything ends up working okay. I think that this might be too far in advance (and the logic here is easy enough that we could fix it later) but something to keep in mind. This could also matter sooner if someone wants to do FLARE with isotopes, to keep track of distinct species.
There was a problem hiding this comment.
I guess we can change the element_to_Z part and allow hyps_mask to take any number beyond 118. I can change the ParameterHelper to accommodate that.
But we need to think a bit how to make it consistent. Maybe leave a dictionary in the element_coder module that allows users to injet str-integer pair. And the element_to_Z and Z_to_element function can read from that dictionary.
|
Thanks for adding those tests and for linting!! (a favor not only to the future version of you but also the future developers and contributors... :) ) |
ParameterHelper failed to apply the right constraints defined by set_constraints() functions, because the order was messed up...
If user use ParameterHelper to generate hyps_mask for constrained hyper-parameter optimization, it will optimized the wrong element of the hyper-parameters...