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

[BUGFIX] expect_day_count_to_be_close_to_equivalent_week_day_mean #7782

Merged

Conversation

HadasManor
Copy link
Contributor

Please annotate your PR title to describe what the PR does, then give a brief bulleted description of your PR below. PR titles should begin with [BUGFIX], [FEATURE], [DOCS], [MAINTENANCE], or [CONTRIB]. If a new feature introduces breaking changes for the Great Expectations API or configuration files, please also add [BREAKING]. You can read about the tags in our contributor checklist.

Changes proposed in this pull request:

  • Fixed bug in expect_day_count_to_be_close_to_equivalent_week_day_mean

  • Deleted old expectation that was replaced by expect_day_count_to_be_close_to_equivalent_week_day_mean (same expectation but better name and with fixed bugs)

  • My code follows the Great Expectations style guide

  • I have performed a self-review of my own code

  • I have commented my code, particularly in hard-to-understand areas

  • I have made corresponding changes to the documentation

  • I have added unit tests where applicable and made sure that new and existing tests are passing.

  • I have run any local integration tests and made sure that nothing is broken.

@netlify
Copy link

netlify bot commented May 1, 2023

Deploy Preview for niobium-lead-7998 canceled.

Name Link
🔨 Latest commit 061ac58
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/645c2452cb67da0008584e4a

@kenwade4
Copy link
Contributor

kenwade4 commented May 3, 2023

Hello @HadasManor can you pull in the latest changes from develop and resolve your merge conflicts. Can you also restore the deleted file? Thanks.

@HadasManor
Copy link
Contributor Author

HadasManor commented May 4, 2023

Hi @kenwade4 , I also wrote you in slack about this, I did pull the latest changes and there are still conflicts so I am not sure what is happening (also the Resolve conflicts button here is greyed out so I cannot even see them).

Regarding the deleted file- as I wrote in the PR description, this is an old expectation that has changed names (to the other file here), and should not have stayed. It is the same, except the deleted file is not updated and contains bugs. For the benefit of our users, it needs to be deleted

@kenwade4
Copy link
Contributor

kenwade4 commented May 4, 2023

@HadasManor the button is greyed out when the conflict cannot be resolved automatically

% git pull

% git merge origin/develop

% vim contrib/experimental/great_expectations_experimental/expectations/expect_day_count_to_be_close_to_equivalent_week_day_mean.py
...
        # get counts for dates
        query = (
<<<<<<< HEAD
            sa.select([sa.func.Date(column), sa.func.count()])
            .group_by(sa.func.Date(column))
            .select_from(selectable)
            .order_by(sa.func.Date(column).desc())
            .limit(METRIC_SAMPLE_LIMIT)
=======
            sa.select(sa.func.Date(column), sa.func.count())
            .group_by(sa.func.Date(column))
            .select_from(selectable)
            .order_by(sa.func.Date(column).desc())
            .limit(30)
>>>>>>> origin/develop
        )
        results = sqlalchemy_engine.execute(query).fetchall()

I'd guess to keep the chunk that uses .limit(METRIC_SAMPLE_LIMIT), delete the other chunk, delete the <<<<< >>>>>> ===== lines

% git add contrib/experimental/great_expectations_experimental/expectations/expect_day_count_to_be_close_to_equivalent_week_day_mean.py

% git rm contrib/experimental/great_expectations_experimental/expectations/expect_yesterday_count_compared_to_avg_equivalent_days_of_week.py

% git commit

You will also need to update the renamed dict here https://github.com/great-expectations/great_expectations/blob/develop/great_expectations/expectations/registry.py#L388-L391 ... put the old Expectation name as the key and the new one as the value.

@HadasManor
Copy link
Contributor Author

HadasManor commented May 7, 2023

I obviously did that, but I don't get the same thing as what you're seeing, that's why I was asking for help :) I keep getting "Already up to date"
image

@kenwade4 kenwade4 enabled auto-merge (squash) May 10, 2023 18:01
@anthonyburdi anthonyburdi merged commit 1ca6ece into great-expectations:develop May 10, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants