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

feature/SOF 7397 #151

Merged
merged 59 commits into from
Aug 12, 2024
Merged

feature/SOF 7397 #151

merged 59 commits into from
Aug 12, 2024

Conversation

VsevolodX
Copy link
Member

@VsevolodX VsevolodX commented Jul 25, 2024

  • feat: add first implementations of perturbation

@VsevolodX VsevolodX changed the base branch from main to feature/SOF-7395 July 27, 2024 02:20
@VsevolodX VsevolodX marked this pull request as ready for review July 31, 2024 03:23

@staticmethod
def deform_slab(configuration):
new_material = configuration.slab.clone()
Copy link
Member

Choose a reason for hiding this comment

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

The logic is ambiguous - why is there slab in configuration for deformation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to SlabDeformationBuilder



class ContinuousDeformationBuilder(DeformationBuilder):
def deform_slab_isometrically(self, configuration):
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

new_basis.to_crystal()
new_material.basis = new_basis

# TODO: adjust the lattice parameters with the same coordinates transformation
Copy link
Member

Choose a reason for hiding this comment

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

Remove or implement

from ...utils import DeformationFunctionBuilder


class DeformationConfiguration(BaseModel, InMemoryEntity):
Copy link
Member

Choose a reason for hiding this comment

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

DeformationConfiguration with deformation_function

SlabDeformationConfiguration inheriting from it and adding slab

Same for builders

]


class DeformationFunctionBuilder:
Copy link
Member

Choose a reason for hiding this comment

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

Builder -> Holder for this and above class

return coordinate


def solve_sine_wave_coordinate_prime(
Copy link
Member

Choose a reason for hiding this comment

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

Docstring

return sine_wave(coordinate_prime, amplitude, wavelength, phase, axis="x")


def sine_wave_radial(
Copy link
Member

Choose a reason for hiding this comment

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

The coordinate functions need to be better organized

@@ -0,0 +1,63 @@
from typing import List
Copy link
Member

Choose a reason for hiding this comment

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

Let's use perturbation

Base automatically changed from feature/SOF-7395 to main July 31, 2024 23:25
return new_material

@staticmethod
def _set_new_coordinates(new_material, new_coordinates):
Copy link
Member

Choose a reason for hiding this comment

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

This can be inside material class or basis class



class SlabPerturbationBuilder(PerturbationBuilder):
def create_perturbed_slab(self, configuration):
Copy link
Member

Choose a reason for hiding this comment

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

Here and elsewhere - type hints



class CellMatchingDistancePreservingSlabPerturbationBuilder(DistancePreservingSlabPerturbationBuilder):
def _transform_cell_vectors(self, configuration: PerturbationConfiguration) -> List[List[float]]:
Copy link
Member

Choose a reason for hiding this comment

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

_transform_lattice_vectors or _transform_cell


class PerturbationConfiguration(BaseModel, InMemoryEntity):
material: Material
perturbation_function: Tuple[Callable, Dict] = PerturbationFunctionHolder.sine_wave()
Copy link
Member

Choose a reason for hiding this comment

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

We need to call things for what they are if this is a tuple, we should call it a tuple, or object, or function_config_tuple

Copy link
Member Author

Choose a reason for hiding this comment

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

I restructured instead

return converted_array.tolist()


class CoordinateConditionBuilder:
Copy link
Member

Choose a reason for hiding this comment

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

Should go to coordinate

)


class PerturbationFunctionHolder:
Copy link
Member

Choose a reason for hiding this comment

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

Should be inside perturbation subfolder

raise NotImplementedError


class SineWave(FunctionHolder):
Copy link
Member

Choose a reason for hiding this comment

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

SineWaveFunctionHolder

) -> Callable[[List[float]], List[float]]:
index = AXIS_TO_INDEX_MAP[axis]

def coordinate_transformation(coordinate: List[float]):
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid nested defs

from mat3ra.utils.factory import BaseFactory


class PerturbationFunctionHelperFactory(BaseFactory):
Copy link
Member

Choose a reason for hiding this comment

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

PerturbationFunctionHolderFactory

@@ -0,0 +1,84 @@
from typing import Callable, List, Literal
Copy link
Member

Choose a reason for hiding this comment

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

file should be called functions

"""
raise NotImplementedError

def apply_derivative(self, coordinate: List[float]) -> float:
Copy link
Member

Choose a reason for hiding this comment

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

calculate_derivative

"""
raise NotImplementedError

def get_arc_length(self, a: float, b: float) -> float:
Copy link
Member

Choose a reason for hiding this comment

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

calculate_arc_length

raise NotImplementedError


class PerturbationFunctionHolder(FunctionHolder):
Copy link
Member

Choose a reason for hiding this comment

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

Should be in perturbation folder

"""
raise NotImplementedError

def get_json(self) -> dict:
Copy link
Member

Choose a reason for hiding this comment

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

Let's check for this to be used instead of the tuple logic in all the previously written code also

@VsevolodX VsevolodX merged commit 361e423 into main Aug 12, 2024
9 checks passed
@VsevolodX VsevolodX deleted the feature/SOF-7397 branch August 12, 2024 17:48
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.

2 participants