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

Sensors fatih #12

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Sensors fatih #12

wants to merge 26 commits into from

Conversation

soenmezmehmet
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@danielandresarcones danielandresarcones left a comment

Choose a reason for hiding this comment

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

I finally got to review the PR. Mostly minor chagens of format, comments, order, etc.
There is one major change to be tackled with API_Request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

pkl objects should not be commited to the repository unless they were strictly necessary for the program. It is also at the root path, which is not desirable.



#

Copy link
Collaborator

Choose a reason for hiding this comment

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

This part is not necessary, the file should only contain the the class. Seems that the api part it just there for debugging.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as the other pkl

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why these test cases are not implemented with unittest? It would work well as a test case where you define the temporal files and the runs the assertions.

import importlib.util
from pathlib import Path

class Unpickler:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Unpickler could be added to the utilities folder of the package, better than here in the tests

''' Base class for a generator of synthetic data from a model.'''

"""
def __init__(self, model_path:str, sensor_positions_path: str, model_parameters: dict, output_parameters: dict = None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commented test?

raise NotImplementedError("export_output should be implemented")

"""
@staticmethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete if not used

@@ -0,0 +1,122 @@
from nibelungenbruecke.scripts.digital_twin_orchestrator.base_model import BaseModel
#from base_model import BaseModel
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete comment

def GenerateData(self):
"""Generate data based on the model parameters."""

self.api_request = API_Request()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We will probably have to supply the secretes path for the API key every time we do API_Request().

translator.translator_to_sensor()

self.problem.import_sensors_from_metadata(self.model_parameters["MKP_meta_output_path"])
self.problem.fields.temperature = self.problem.fields.displacement #!!
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this? Fenicsxconcrete supports temperature fields. Additionally, displacements are a vector a field and temperature a scalar, they should not be initialized as the same. Temperature is not even needed for this model.

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