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.

Sign up for GitHub

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

Jump to bottom

Feature/dei 109 duration analysis group #40

Merged
merged 8 commits into from
Aug 17, 2023
Merged

Feature/dei 109 duration analysis group #40

merged 8 commits into from
Aug 17, 2023

Conversation

CindyvdVries
Copy link
Collaborator

  • Added two extra operation types for the time aggregation rule: MAX_DURATION_PERIOD and AVG_DURATION_PERIOD
  • Improved the functionality for COUNT_PERIODS
  • Updated the tests
  • Fixed a major bug in the way the reduce function looped over the aggregated values

@mKlapwijk mKlapwijk self-requested a review August 8, 2023 14:27
mKlapwijk
mKlapwijk requested changes Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@mKlapwijk mKlapwijk left a comment

Choose a reason for hiding this comment

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

Some remarks for clarification

decoimpact/business/entities/rules/time_aggregation_rule.py
# count the occurences of difference=1, and then add the first value:
# (the result of this examples: 1 + 1 = 2)
differences = _np.append(differences, elem[0])
return _np.count_nonzero(differences == 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the description says that the first value is added after counting the occurences. The code does the opposite, first append, then count. Which is correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, the code was changed and forgot to update the text. The first element of the array is either 0 or 1, so it is an addition to the sum, could be done first or second, but because I made a switch to use count_nonzero I moved the adding to the line above. Will adjust the comment

decoimpact/business/entities/rules/time_aggregation_rule.py Outdated

def duration_groups(self, elem):
"""
Count the amount of times the groups of 1 occur.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the description up to date with the name of the function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, geupdate

decoimpact/business/entities/rules/time_aggregation_rule.py
List: List with the counted periods
"""
# Function to create a cumsum over the groups (where the elements in elem are 1)
cumsum_groups = _np.frompyfunc(lambda a, b: a + b if b == 1 else 0, 2, 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

leuke functie, maar ik snap het niet goed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added an example in the description of the duration_groups function explaining what the result is of this function. a and b are consecutive values when looping over the array in this function.

decoimpact/business/entities/rules/time_aggregation_rule.py Outdated
return cumsum_groups.accumulate(elem)

def analyze_groups(self, elem, axis, **kwargs):
"""In an array with 0 and 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

description seems to be incorrect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

decoimpact/business/entities/rules/time_aggregation_rule.py
no_axis = len(_np.shape(elem))

if axis != -1:
elem = _np.moveaxis(elem, axis, -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we do this exactly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll put in a comment

decoimpact/business/entities/rules/time_aggregation_rule.py
for sub_elem in elem:
# loop through this recursive function, determine output per axis:
group_count_row = self.count_groups(sub_elem, axis - 1)
group_result_row = self.analyze_groups(sub_elem, axis)
Copy link
Collaborator

Choose a reason for hiding this comment

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

right, I think I understand it a bit more

@CindyvdVries
Copy link
Collaborator Author

I added a lot of proza in comments, hopefully to make it more understandable!

IoannaMi
IoannaMi approved these changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@IoannaMi IoannaMi left a comment

Choose a reason for hiding this comment

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

Looks good!

@CindyvdVries CindyvdVries merged commit 57b5557 into main Aug 17, 2023
@CindyvdVries CindyvdVries deleted the feature/DEI-109-duration-analysis-group branch August 17, 2023 09:09
MPWeeber added a commit that referenced this pull request Sep 11, 2023
@MPWeeber
Squashed commit of the following:
c8b1ab4 
commit 545baab
Merge: a289bd7 6f19da4
Author: Wouter Schoonveld <35798126+wschoonveld@users.noreply.github.com>
Date:   Tue Aug 29 10:39:50 2023 +0200

    Merge pull request #50 from Deltares/feature/DEI-118-time-filter-on-all-variables

    Feature/dei 118 time filter on all variables

commit 6f19da4
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Thu Aug 24 20:18:58 2023 +0200

    feat[DEI-118] update docs and clean up code

commit 2b9e014
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Thu Aug 24 20:14:04 2023 +0200

    feat[DEI-118]: extend unit test for time filter with end date

commit 6963ab9
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Thu Aug 24 20:05:30 2023 +0200

    feat[DEI-118]: fake problems fixed

commit 044aa57
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Thu Aug 24 17:40:53 2023 +0200

    feat[DEI-118]: fix unit test to test time filter on start_date

commit e6f0c86
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Wed Aug 23 09:46:09 2023 +0200

    feat[DEI-118]: unit test added to test time filter

commit 076bf9f
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Tue Aug 22 14:55:31 2023 +0200

    feat[DEI-118]: update unit test for test_dataset_data for adding time filter

commit 931de7a
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Tue Aug 22 14:35:25 2023 +0200

    feat[DEI-118]: log message when applying time filter and only apply when given

commit 7c8eea0
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Tue Aug 22 14:18:38 2023 +0200

    feat[DEI-118]: make time filter optional

commit fd46e53
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Tue Aug 22 10:38:48 2023 +0200

    feat[DEI-118]: accept end_date as time filter from input file

commit c8cff61
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Tue Aug 22 10:05:36 2023 +0200

    feat[DEI-118]: pass start_date as parameter from input yaml

commit a289bd7
Merge: 73e72dc 217611c
Author: mKlapwijk <48097248+mKlapwijk@users.noreply.github.com>
Date:   Mon Aug 21 10:07:38 2023 +0200

    Merge pull request #49 from Deltares/feature/DEI-117-filter-variable-on-time

    Feature/DEI-117 filter variable on time

commit 5f6e70c
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Fri Aug 18 17:30:36 2023 +0200

    feat[DEI-118]: apply time filter on input dataset

commit 217611c
Author: CindyvdVries <cindyvdvries@gmail.com>
Date:   Fri Aug 18 13:37:28 2023 +0200

    [DEI-117]: fix unit tests

commit 73e72dc
Merge: 9d5c1b8 4f69a35
Author: mKlapwijk <48097248+mKlapwijk@users.noreply.github.com>
Date:   Fri Aug 18 13:32:36 2023 +0200

    Merge pull request #48 from Deltares/feature/DEI-121-installation-issues-miniconda

    feat[DEI-121]: update readme to avoid issues with installation of pac…

commit 4533188
Author: CindyvdVries <cindyvdvries@gmail.com>
Date:   Fri Aug 18 13:04:54 2023 +0200

    fix [DEI-117]: update from review comments

commit 4f69a35
Author: Hidde Elzinga <Hidde.Elzinga@deltares.nl>
Date:   Fri Aug 18 13:04:19 2023 +0200

    Small update about the env_name

    Added missing argument

commit 9d5c1b8
Merge: 57b5557 eb91916
Author: HiddeElzinga <65541435+HiddeElzinga@users.noreply.github.com>
Date:   Thu Aug 17 15:28:53 2023 +0200

    Merge pull request #44 from Deltares/feat-DEI-116]-Fix-readme.md-and-header-in-couple-of-missing-files

    feat[DEI-116] fixed readme.md and minor errors in (missing) headers o…

commit 6fdc4de
Author: CindyvdVries <cindyvdvries@gmail.com>
Date:   Thu Aug 17 15:21:09 2023 +0200

    [DEI-117]: fix flake8

commit 146b66f
Author: IoannaMi <ioannamicha@hotmail.com>
Date:   Thu Aug 17 13:43:23 2023 +0200

    feat[DEI-121]: update readme to avoid issues with installation of packages

commit b92a091
Author: CindyvdVries <cindyvdvries@gmail.com>
Date:   Thu Aug 17 13:08:28 2023 +0200

    [DEI-117]:  update mkdocs

commit fac4a26
Author: CindyvdVries <cindyvdvries@gmail.com>
Date:   Thu Aug 17 12:49:52 2023 +0200

    [DEI-117]: add acceptance test

commit 4d9c7d5
Author: CindyvdVries <cindyvdvries@gmail.com>
Date:   Thu Aug 17 11:50:31 2023 +0200

    [DEI-117]: add unit tests

commit 3cce885
Author: CindyvdVries <cindyvdvries@gmail.com>
Date:   Thu Aug 17 11:50:19 2023 +0200

    [DEI-117]: Bug fix, date_range was not correctly given to rule

commit 5704bd4
Author: CindyvdVries <cindyvdvries@gmail.com>
Date:   Thu Aug 17 11:49:50 2023 +0200

    [DEI-117]: fix bug overlapping periods and where no periods defualt = nan

commit 57b5557
Author: CindyvdVries <cindyvdvries@gmail.com>
Date:   Thu Aug 17 11:09:41 2023 +0200

    Feature/dei 109 duration analysis group (#40)

    * DEI-109: first implementation of max/avg period analyze

    * [DEI-109]: update first tests with new operation types

    * [DEI-109]: fix looping over correct axis

    * [DEI-109]: add comments to clarify changes (update from review)

    * flake8..

    * fix [DEI-109]: division by 0 periods and added tests

commit eb91916
Author: David Rodriguez Aguilera <32545627+Davidrag@users.noreply.github.com>
Date:   Thu Aug 17 09:25:09 2023 +0200

    feat[DEI-116] fixed readme.md and minor errors in (missing) headers of other files.

commit 2b60bc7
Merge: a9c8f75 fb6628e
Author: mKlapwijk <48097248+mKlapwijk@users.noreply.github.com>
Date:   Wed Aug 16 15:50:35 2023 +0200

    Merge pull request #43 from Deltares/feat-DEI-116]-Apply-licensing-+-copyright-to-all-source-files,-the-README-and-License-file

    feat[DEI-10} Changed case of installation.md

commit fb6628e
Author: David Rodriguez Aguilera <32545627+Davidrag@users.noreply.github.com>
Date:   Wed Aug 16 15:44:56 2023 +0200

    feat[DEI-116] added header to test files

    Extension "Gruntfuggly.auto-snippet" must be added.

commit c6e98d2
Author: David Rodriguez Aguilera <32545627+Davidrag@users.noreply.github.com>
Date:   Wed Aug 16 15:39:25 2023 +0200

    feat[DEI-116] added automatically generated header for new *.py files

    Extension "Gruntfuggly.auto-snippet" must be added.

commit 3a34288
Author: David Rodriguez Aguilera <32545627+Davidrag@users.noreply.github.com>
Date:   Wed Aug 16 15:24:49 2023 +0200

    feat[DEI-116] added "License" and version number to the header

commit 49e99d8
Author: David Rodriguez Aguilera <32545627+Davidrag@users.noreply.github.com>
Date:   Wed Aug 16 15:24:11 2023 +0200

    Revert "feat[DEI-116] added "License" and version number to the header"

    This reverts commit 9aeba52.

commit 9aeba52
Author: David Rodriguez Aguilera <32545627+Davidrag@users.noreply.github.com>
Date:   Wed Aug 16 15:23:23 2023 +0200

    feat[DEI-116] added "License" and version number to the header

commit 8ce6706
Author: David Rodriguez Aguilera <32545627+Davidrag@users.noreply.github.com>
Date:   Wed Aug 16 13:12:35 2023 +0200

    feat[DEI-116] Shortened line with link (too many characters)

commit cfc6b3a
Author: David Rodriguez Aguilera <32545627+Davidrag@users.noreply.github.com>
Date:   Wed Aug 16 11:56:59 2023 +0200

    feat[DEI-10} Changed case of installation.md

commit a9c8f75
Author: CindyvdVries <cindyvdvries@gmail.com>
Date:   Tue Aug 15 08:07:54 2023 +0200

    Poc/dei 113 pytest acceptance test (#42)

    * add acceptance test script

    * readd test

    * Move acceptance test file, and update poetry accordingly so VS code can find it

    * Get test_main working

    * Add test files

    * Update gitignore for test data

    * correct file path

    * Convert from runpy to subprocess

    * update git ignore and add reference files location

    * Revert "update git ignore and add reference files location"

    This reverts commit 5b185ed.

    * Add reference files location

    * Update git girnore for reference results

    * Add reference comparison, first step

    * rename test case and variables in pytest

    * rename reference result

    * DEI-115: add output folder

    * DEI-113: pass environment to enable import xarray

    * DEI-113: pass running python interpreter

    * DEI-113: fixing space and missing explanatory function string

    * DEI-113: fix formatting errors

    * DEI-113: attempt to fix flake8 warning

    * DEI-113: fix flake8 error (by using isinstance)

    * feat[DEI-113]: small fixes based on review

    * feat[DEI-113]: add documentation on how to add acceptance tests

    * DEI-113: create output folder if not exists (update from test feedback)

    ---------

    Co-authored-by: mKlapwijk <maarten.klapwijk@deltares.nl>
    Co-authored-by: Wouter <wouter.schoonveld@deltares.nl>
    Co-authored-by: IoannaMi <ioannamicha@hotmail.com>

commit 82c6f59
Author: David Rodriguez Aguilera <32545627+Davidrag@users.noreply.github.com>
Date:   Tue Aug 15 08:06:41 2023 +0200

    Feat dei 10] update where necessary the collaboration document and bring it online (#41)

    * feat[DEI-10] Testing new location for images in subfolder where md is located

    * feat[DEI-10] Testing new location for images in subfolder where md is located

    * feat[DEI-10] Fixing relative path to images in md's

    * feat[DEI-10] Fixied relative path to images in md's

    * feat[DEI-10] First draft of document.

    * feat[DEI-10] Finished formatting and adding figures original text.

    * feat[DEI-10] Finished making images anonymous (data protection)

    * feat[DEI-10] Updated project name.

    * feat[DEI-10] Added some modifications in the text.

    * feat[DEI-10} Added nav for development documantation

    Also changed name of installation.md in order to change case in a later commit.

    * feat[DEI-10} Changed case of installation.md

commit 5390e55
Merge: 2ee2cd2 14a18ac
Author: mKlapwijk <48097248+mKlapwijk@users.noreply.github.com>
Date:   Mon Aug 14 10:57:42 2023 +0200

    Merge pull request #39 from Deltares/feature/DEI-56-fix-enum-conversion-for-python-3.11

    Fix for enum conversion for python 3.11, works also for python 3.9

commit 14a18ac
Author: HiddeElzinga <Hidde.Elzinga@deltares.nl>
Date:   Thu Apr 6 17:52:36 2023 +0200

    poc[DEI-56]: Add possible fix for enum conversion

commit 2ee2cd2
Merge: 6e0afa2 7734e52
Author: mKlapwijk <48097248+mKlapwijk@users.noreply.github.com>
Date:   Fri Aug 4 15:43:59 2023 +0200

    Merge pull request #38 from Deltares/feature/DEI-115-table-format-response-curve-parser

    feat[DEI-115]: change response curve parser to read response table

commit 7734e52
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Thu Aug 3 13:59:24 2023 +0200

    feat[DEI-115]: tutorial response_curve updated

commit 4d16cbe
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Wed Aug 2 13:06:36 2023 +0200

    feat[DEI-115]: fixed test for length response_table

commit d37b5d7
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Wed Aug 2 12:26:24 2023 +0200

    feat[DEI-115]: fix formatting (missing white spaces)

commit 0099a2a
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Wed Aug 2 12:14:04 2023 +0200

    feat[DEI-115]: fix formatting (test columns response_table)

commit 8aef231
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Wed Aug 2 12:08:10 2023 +0200

    feat[DEI-115]: unit test added for number of columns response_table

commit 903bc7a
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Wed Aug 2 11:50:15 2023 +0200

    feat[DEI-115]: cleanup comments

commit b38720f
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Wed Aug 2 11:16:01 2023 +0200

    feat[DEI-115]: repsonse_table: unit tests fixed

commit cc0ff0f
Author: Wouter <wouter.schoonveld@deltares.nl>
Date:   Wed Aug 2 08:55:19 2023 +0200

    feat[DEI-115]: change response curve parser to read response table
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mKlapwijk mKlapwijk mKlapwijk requested changes

@IoannaMi IoannaMi IoannaMi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CindyvdVries @mKlapwijk @IoannaMi