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

Implement measurement field conversion from Vision #240

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

zhen0427
Copy link
Contributor

@zhen0427 zhen0427 commented Apr 4, 2024

only part of the code to determine the measured terminal type is finished. Further changes in the tabular converter is not implemented. Therefore pytest is still failed.

Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left a comment

Choose a reason for hiding this comment

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

Not sure if ready for review, but since it's mentioned in the meeting today.

@@ -119,4 +119,4 @@ def __getitem__(self, idx: int) -> Any:
Returns:
The original item
"""
return self._items[idx]
return getattr(self._items[idx], None)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this change? pytest fails this one and passes when reverted to the original. (It should also be supplied with a str for the name field)

Copy link
Member

Choose a reason for hiding this comment

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

i indeed think that this is not what you intended to do. as it is now, besides the missing str type, it will basically try to seek the attribute None in self._items[idx], e.g. self._items[idx].None

My guess is that you want to make it such that you return None if idx is not in self_items? in that case, you will want to use

Suggested change
return getattr(self._items[idx], None)
return self._items.get(idx)

which is shorthand for

        return self._items.get(idx, None)

(the None is the default argument)

Comment on lines 137 to 139
if id is not None:
return MEASURED_TERMINAL_TYPE_MAP[key]
_LOG.warning("No measured terminal type is found!")
Copy link
Member

Choose a reason for hiding this comment

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

what happens when key is not in MEASURED_TERMINAL_TYPE_MAP?

@@ -119,4 +119,4 @@ def __getitem__(self, idx: int) -> Any:
Returns:
The original item
"""
return self._items[idx]
return getattr(self._items[idx], None)
Copy link
Member

Choose a reason for hiding this comment

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

i indeed think that this is not what you intended to do. as it is now, besides the missing str type, it will basically try to seek the attribute None in self._items[idx], e.g. self._items[idx].None

My guess is that you want to make it such that you return None if idx is not in self_items? in that case, you will want to use

Suggested change
return getattr(self._items[idx], None)
return self._items.get(idx)

which is shorthand for

        return self._items.get(idx, None)

(the None is the default argument)

@nitbharambe
Copy link
Member

@zhen0427 I suppose this function belongs to phase_to_phase functions
A test also needs to be in tests\unit\functions\test_phase_to_phase.py

Signed-off-by: Zhen Wang <[email protected]>
@@ -242,26 +251,33 @@ def _convert_col_def_to_attribute(
"""
# To avoid mistakes, the attributes in the mapping should exist. There is one extra attribute called
# 'extra' in which extra information can be captured.
if attr not in pgm_data.dtype.names and attr != "extra":
if attr not in pgm_data.dtype.names and attr not in ["extra", "filter"]:
Copy link
Member

Choose a reason for hiding this comment

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

pro tip: use () for inline things. it's one of the few cases where the python interpreter can optimize during parsing because tuples () are fixed-size. it's minor in this case, but this stuff accumulates multiplicatively so it can add up

Suggested change
if attr not in pgm_data.dtype.names and attr not in ["extra", "filter"]:
if attr not in pgm_data.dtype.names and attr not in ("extra", "filter"):

@@ -517,7 +533,7 @@ def _parse_auto_id(
key_col_def: A column definition which should be unique for each object within the current table

Returns: A single column containing numerical ids

Copy link
Member

Choose a reason for hiding this comment

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

probably the formatter is already complaining so just calling it out

Suggested change


if len(attr_data.columns) != 1:
raise ValueError(f"DataFrame for {component}.{attr} should contain a single column ({attr_data.columns})")

pgm_data[attr] = attr_data.iloc[:, 0]

def _parse_filters() -> pd.Series[bool]:
mask = True * len(data) * 483
Copy link
Member

Choose a reason for hiding this comment

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

  • why specifically 483? please name this (so-called) magic value
  • i believe this doesn't do what you want because the current mask produces a scalar value, not a function or multi-dimensional array

Comment on lines 148 to 150

def filter_if_object(object_name: str, excl_object: str) -> bool:
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 in other places: please consistently use 2 whitelines when there is no indentation, and 1 whiteline if there is indentation. e.g.since def filter_if_object is not indented, this should be prefixed with 2 whitelines

Suggested change
return input_string != "link"
def filter_if_object(object_name: str, excl_object: str) -> bool:
return input_string != "link"
def filter_if_object(object_name: str, excl_object: str) -> bool:

Copy link
Contributor

Choose a reason for hiding this comment

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

These two are output files and it's generally not necessary to be version them.

Copy link
Contributor

Choose a reason for hiding this comment

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

These two are output files and it's generally not necessary to be version them.

@@ -67,14 +67,14 @@ def test_set_mapping_file(converter: TabularConverter):
with pytest.raises(ValueError, match="Mapping file should be a .yaml file, .txt provided."):
converter.set_mapping_file(mapping_file=Path("dummy/path.txt"))

dummy_path = Path(__file__).parents[2] / "data" / "config" / "dummy_mapping.yaml"
dummy_path = Path(__file__).parents[2] / "data" / "config" / "dummy.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no dummy.yaml in the directory, instead there is a mapping.yaml

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.

4 participants