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

[GSK-2248] TestResult to dict #9

Merged
merged 3 commits into from
Dec 14, 2023
Merged

[GSK-2248] TestResult to dict #9

merged 3 commits into from
Dec 14, 2023

Conversation

rabah-khalek
Copy link
Contributor

No description provided.

Copy link

linear bot commented Dec 13, 2023

GSK-2248 Generate JSON output from the tests

This should be the last step of the short-term milestone. We should get feedback from Alex on this to finalise the format.

@rabah-khalek rabah-khalek marked this pull request as draft December 13, 2023 14:27
@rabah-khalek rabah-khalek requested a review from Hartorn December 14, 2023 14:08
@rabah-khalek rabah-khalek marked this pull request as ready for review December 14, 2023 14:09
self._wrapped_dataloader = dataloader
self.name = name
Copy link
Member

Choose a reason for hiding this comment

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

Not needed, any call to .name would be redirected to self._wrapped_dataloader.name, using __getattr__

Or you could add a func with @property.getter, to add some description like :

@property.getter
def name(self):
  return f"Wrapper({self._wrapped_dataloader.name}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -16,7 +16,8 @@ def __init__(
crop_img: bool = True,
crop_marks: bool = True,
) -> None:
super().__init__(dataloader)
name = f"Cropped on {part.name}"
Copy link
Member

Choose a reason for hiding this comment

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

Kind of same as for the base class, could be a method

@property.getter
def name(self):
  return f"{self._wrapped_dataloader.name} cropped on {self._part}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -46,7 +47,7 @@ def __getitem__(self, idx: int) -> Tuple[np.ndarray, Optional[np.ndarray], Optio

class CachedDataLoader(DataLoaderWrapper):
def __init__(self, dataloader: DataIteratorBase, cache_size: int = 20) -> None:
super().__init__(dataloader)
super().__init__(dataloader, name="Cached")
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

str(round(self.threshold, 2)),
f"Prediction fail rate: {FR}" if FR != 0 else "",
str(round(self.prediction_time, 2)),
)

def to_dict(self):
return {
Copy link
Member

Choose a reason for hiding this comment

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

Could use dataclasses.asdict instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I don't need all the attributes of TestResult to be included. But yeah, probably the remaining ones could be eliminated in the future if we don't need them, and to_dict can be therefore also eliminated in favour of asdict. Thanks!

@rabah-khalek rabah-khalek requested a review from Hartorn December 14, 2023 14:49
@rabah-khalek rabah-khalek merged commit 35adb52 into main Dec 14, 2023
1 of 3 checks passed
@rabah-khalek rabah-khalek deleted the GSK-2248 branch December 14, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants