Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

Refactoring #77

Merged
merged 34 commits into from
Aug 30, 2023
Merged

Refactoring #77

merged 34 commits into from
Aug 30, 2023

Conversation

leonlan
Copy link
Owner

@leonlan leonlan commented Aug 22, 2023

Closes #58. This PR:

  • Introduces a single Environment with classmethods for the EURO-NeurIPS variant as well as our paper variant.
  • Introduces the SamplingMethod protocol.
  • Introduces the ConsensFunction protocol.
  • Removes iteration-specific thresholds.

@leonlan leonlan requested a review from N-Wouda August 29, 2023 13:42
@leonlan
Copy link
Owner Author

leonlan commented Aug 29, 2023

@N-Wouda Could you have a quick look at this? You don't have to look at the details, just a high-level review of the main points (listed in the PR) would be already enough!

@N-Wouda
Copy link
Collaborator

N-Wouda commented Aug 29, 2023

I'll have a look at this in a minute!

Copy link
Collaborator

@N-Wouda N-Wouda left a comment

Choose a reason for hiding this comment

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

LGTM, few comments. I didn't look at the Environment in great detail, but it seems to do what it should do so no comments there.

sampling/euro_neurips.py Outdated Show resolved Hide resolved
benchmark.py Outdated Show resolved Hide resolved
benchmark.py Outdated Show resolved Hide resolved
# object's consensus parameters.
#
# Using these arguments, the consensus function should return which requests
# to dispatch and which to postpone.
CONSENSUS: dict[str, Callable] = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this have values of type ConsensusFunction?

Copy link
Owner Author

Choose a reason for hiding this comment

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

mypy complains about this:

agents/consensus/__init__.py:7: error: Dict entry 0 has incompatible type "str": "Callable[[List[Tuple[Dict[Any, Any], List[List[int]]]], Dict[Any, Any], Any, Any, float, float], Tuple[Any, Any]]"; expected "str": "ConsensusFunction"  [dict-item]
agents/consensus/__init__.py:8: error: Dict entry 1 has incompatible type "str": "Callable[[List[Tuple[Dict[Any, Any], List[List[int]]]], Dict[Any, Any], Any, Any, float], Any]"; expected "str": "ConsensusFunction"  [dict-item]
agents/consensus/__init__.py:9: error: Dict entry 2 has incompatible type "str": "Callable[[List[Tuple[Dict[Any, Any], List[List[int]]]], Any, Any, Any, float, int, float], Tuple[Any, Any]]"; expected "str": "ConsensusFunction"  [dict-item]

I couldn't really figure out how to get this to work. Tried to add **kwargs to ConsensusFunction but that didn't work either. I'm OK with this now, at least it's a bit more explicit then what I had before :-)

@leonlan leonlan changed the title Refactor environments Refactoring Aug 30, 2023
@leonlan leonlan merged commit 088e79d into master Aug 30, 2023
1 check passed
@leonlan leonlan deleted the refactor-env branch August 30, 2023 06:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring
2 participants