Skip to content
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

Molwitch chirality #332

Merged
merged 6 commits into from
May 10, 2024
Merged

Molwitch chirality #332

merged 6 commits into from
May 10, 2024

Conversation

ChemMitch
Copy link
Collaborator

In StructureProcessor, look for parameters that can be passed indirectly to a molwitch ChemicalImpl object

Copy link

@tylerperyeaFDA tylerperyeaFDA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue here is just that we're re-loading and re-applying the parameters from the config whenever we are processing a structure. This is a problem for 2 reasons:

  1. It's unnecessary, we just need to call it once.
  2. It may not have been called in the right order. The renderer code doesn't use this stuff but still relies on these properties having been set in the past.

To remedy this we should add a new configuration class to read all chemical props, and then have the existing molwitchLoader class use that configuration class to apply params. That way it only happens once, it happens at the beginning, and it's future-proof to take any new parameter a molwitch implementation may allow without having to explicitly enumerate them.

@@ -133,6 +146,30 @@ void instrument (StructureProcessorTask settings) {
Structure struc = settings.getStructure();
Collection<Structure> components = settings.getComponents();
Chemical mol = settings.getChemical().copy();
if( maxUndefined !=null || complexityCutoff !=null ){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be here ... it should be a global setting that happens when the application starts, since it's changing static settings once, right?

@Slf4j
@Component
@Configuration
@ConfigurationProperties("gsrs.structure.processing")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just make a new class for this configuration, and make it so that it parses everything found under:

gsrs.structure.chemical.molwitch

into a Map<String, Object>. That is:

@ConfigurationProperties("gsrs.structure.chemical")
@Data
public class ChemicalParsingProperties
...
 private Map<String,Object> molwitch;
 
 

And then in MolwitchLoader add a ChemicalParsingProperties autowired bean and then, at the end of invokeMolwitch do:

...
Map<String,Object> params = chemProps.getMolwitch();
ChemicalImplFactory defFAC = ImplUtil.getChemicalImplFactory();
defFAC.applyParameters(params);

Then we'll only be calling this once on load, and it'll be ready for everything, including the renderer (which may bypass the structure processor).

Copy link

@tylerperyeaFDA tylerperyeaFDA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just get rid of one unnecessary line, then we're good

@@ -27,6 +27,8 @@ public class StructureProcessor {

private StructureHasher hasher;

private List<String> processedFactoryClasses = new ArrayList<>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

@ChemMitch ChemMitch merged commit fbbc485 into master May 10, 2024
2 of 3 checks passed
@blueSwordfish blueSwordfish deleted the molwitch_chirality branch September 17, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants