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

Split mapping #127

Merged
merged 21 commits into from
Oct 5, 2023
Merged

Split mapping #127

merged 21 commits into from
Oct 5, 2023

Conversation

LadyChristina
Copy link
Member

@LadyChristina LadyChristina commented Sep 5, 2023

All Submissions:

  • Have you followed the guidelines in our Contributing documentation?
  • Have you verified that there aren't any other open Pull Requests for the same update/change?
  • Does the Pull Request pass all tests?

Description

Split "mapping" into two modules, mapping and aggregation. Now the mapping is only responsible for mapping blocks to entities (and is not dependent on any timeframe), while the aggregator generates the files with information about the distribution of resources to entities for some timeframe.

Also replaced file reading / writing method (json and csv methods instead of simple string parsing) for clarity and easier testing / maintenance.

Left to do:

  • Add content to mapping tests
  • Update documentation

@LadyChristina LadyChristina changed the title [wip] Split mapping Split mapping Sep 26, 2023
@LadyChristina LadyChristina mentioned this pull request Oct 2, 2023
5 tasks
Copy link
Member

@dimkarakostas dimkarakostas left a comment

Choose a reason for hiding this comment

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

LGTM in general - fix the small issues and I'll approve and merge afterwards

self.aggregated_data_dir = io_dir / 'blocks_per_entity'
self.aggregated_data_dir.mkdir(parents=True, exist_ok=True)

def aggregate(self, timeframe):
Copy link
Member

Choose a reason for hiding this comment

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

I find it a bit confusing to name both functions in the same file aggregate. You could rename this one process. Don't have strong feelings though, so leaving it like that is also fine.

@@ -20,7 +19,7 @@ def map_from_known_identifiers(self, block):
"""
block_identifier = block['identifiers']
day = block['timestamp'][:10]
pool_links = hlp.get_pool_links(self.project_name, day)
pool_links = hlp.get_pool_links(self.project_name, day) # todo move out of method after updating cardano data
Copy link
Member

Choose a reason for hiding this comment

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

What is this todo about? I suggest you open a gh issue instead.

@@ -0,0 +1,18 @@
# Aggregator
Copy link
Member

Choose a reason for hiding this comment

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

You need to activate this page in mkdocs.yml as well.

@LadyChristina LadyChristina merged commit 6fefeca into main Oct 5, 2023
1 check passed
@LadyChristina LadyChristina deleted the split_mapping branch October 6, 2023 09:45
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