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

Correlations: Remove top_n argument #105

Open
wants to merge 64 commits into
base: main
Choose a base branch
from

Conversation

fredmanglis
Copy link
Contributor

The top_n argument is used to pick the top n results of the computation, but in this function, it is way too early to do that, since you need to run filters against the computation results before you pick the results.

  • gn3/computations/rust_correlation.py: Remove top_n argument

fredmanglis and others added 30 commits November 3, 2022 13:31
* gn3/auth/authentication.py: new function `credentials_in_database`
* gn3/auth/authentication/__init__.py: replace package with module
* gn3/settings.py: new `AUTH_MIGRATIONS` configuration variable
* migrations/auth/20221103_02_sGrIs-create-user-credentials-table.py: new
  migration
* tests/unit/auth/test_credentials.py: test the `credentials_in_database`
  function
* tests/unit/conftest.py: more test fixtures
* tests/unit/auth/conftest.py: add fixtures specific to auth
* tests/unit/auth/test_migration_create_users_table.py: import from new
  fixtures module
* tests/unit/conftest.py: remove auth-specific fixtures from *ALL* unit tests
  fixture module.
* tests/unit/auth/test_create_user_credentials_table.py: new tests
* gn3/auth/authentication.py: Fix issues caught by tests
* tests/unit/auth/test_credentials.py: Add fixtures and tests for credentials
  checking
* main.py: Provide the `apply-migrations` CLI command to run the migrations
  against the auth database.

  The command can be invoked with:

      $ flask apply-migrations
* gn3/settings.py: Omit trailing slash
* tests/unit/auth/test_create_table_migrations.py: Generalise testing
  migrations that create tables.
* tests/unit/auth/test_create_user_credentials_table.py: delete
* tests/unit/auth/test_migration_create_users_table.py: delete
* migrations/auth/20221108_02_wxTr9-create-privileges-table.py: new migration
* tests/unit/auth/test_create_table_migrations.py: test new migration
* migrations/auth/20221108_03_Pbhb1-create-resource-categories-table.py: new
  migration.
* tests/unit/auth/test_create_table_migrations.py: test new migration.
* migrations/auth/20221108_04_CKcSL-init-data-in-resource-categories-table.py:
  new migration.
* tests/unit/auth/test_migration_init_data_in_resource_categories_table.py:
  test new migration.
* tests/unit/auth/test_migration_init_data_in_resource_categories_table.py:
  Test that the data is initialised properly. Test that rollback works as
  expected.
selected

This is because base_samples was set to all_samples_ordered, which only
includes primary samples + parents/f1s. Setting this to an empty list
fixed the issue and caused it to use all samples again.
* gn3/migrations.py: Minor change
* migrations/auth/20221110_01_WtZ1I-create-resources-table.py: new migration
* tests/unit/auth/test_create_table_migrations.py: test new migration
* migrations/auth/20221110_02_z1dWf-create-mrna-resources-table.py: new
  migration
* tests/unit/auth/test_create_table_migrations.py: test for new migration
* migrations/auth/20221110_03_ka3W0-create-phenotype-resources-table.py: new
  migration
* tests/unit/auth/test_create_table_migrations.py: test for new migration
* migrations/auth/20221110_04_6PRFQ-create-genotype-resources-table.py: new
  migration
* tests/unit/auth/test_create_table_migrations.py: test new migration
* migrations/auth/20221110_05_BaNtL-create-roles-table.py: new migration
* tests/unit/auth/test_create_table_migrations.py: test new migration
* tests/unit/auth/test_migrations_indexes.py: test new migration
* migrations/auth/20221110_06_Pq2kT-create-generic-roles-table.py: new
  migration
* tests/unit/auth/test_create_table_migrations.py: test new migration
* tests/unit/auth/test_create_table_migrations.py: new migration
* tests/unit/auth/test_migrations_indexes.py: test new migration
* migrations/auth/20221110_07_7WGa1-create-role-privileges-table.py: new
  migration

  Commit ee72678 only contains tests for this
  migration.
* migrations/auth/20221110_08_23psB-add-privilege-category-and-privilege-description-columns-to-privileges-table.py:
  new migration
* tests/unit/auth/test_migrations_add_remove_columns.py: test new migration
* .gitignore: ignore all yoyo configuration files
* README.md: Update documentation
* yoyo.auth.ini: stop tracking the yoyo configuration file.
* migrations/auth/20221113_01_7M0hv-enumerate-initial-privileges.py: new
  migration.
* tests/unit/auth/test_migrations_insert_data_into_empty_table.py: test new
  migration.
Add table `generic_role_privileges` table to link the generic roles to the
privileges they provide.

* migrations/auth/20221114_01_n8gsF-create-generic-role-privileges-table.py:
  new migration
* tests/unit/auth/test_create_table_migrations.py: test new migration
* tests/unit/auth/test_migrations_indexes.py: test new migration
fredmanglis and others added 23 commits November 15, 2022 03:59
* gn3/auth/authorisation/__init__.py: delete function
* gn3/auth/authorisation/checks.py: move function to `checks` module
Use specified types for privileges, roles and types rather than using strings
to help with limiting bugs.

* gn3/auth/authorisation/groups.py: Specify and use the `Group` type
* gn3/auth/authorisation/privileges.py: Specify and use the `Privilege` type
* gn3/auth/authorisation/roles.py: Specify the `Role` type. Add the
  `create_role` function.
* gn3/auth/authorisation/checks.py: Return results of calling the function
  rather than a dict of values that include the results.
* gn3/auth/authorisation/groups.py: Use the newer form of `authorised_p`
  decorator.
* tests/unit/auth/test_groups.py: Update tests
* gn3/db/rdf.py: Delete gn3.setting.SPARQL_ENDPOINT import.
(sparql_query): Inject SPARQLWrapper.
(get_dataset_metadata): Ditto.
* tests/unit/auth/test_roles.py: new tests.
* gn3/auth/authorisation/privileges.py: Set id to UUID type
* gn3/auth/authorisation/roles.py: fix parameters to types that sqlite3
  supports
* gn3/auth/db.py: add logging for errors and re-raise the exception
* tests/unit/auth/test_roles.py: fix test
* migrations/auth/20221116_01_nKUmX-add-privileges-to-group-leader-role.py:
  new migration to fix data errors.
* tests/unit/auth/test_privileges.py: test privileges
* migrations/auth/20221117_01_RDlfx-modify-group-roles-add-group-role-id.py:
  new migration
* tests/unit/auth/test_migrations_add_remove_columns.py: test new migration
* gn3/auth/authentication.py -> gn3/auth/authentication/__init__.py: Convert
  module to package
* gn3/auth/authentication/users.py: Define the `User` type
* tests/unit/auth/conftest.py: Add fixtures to help with testing
* tests/unit/auth/test_groups.py: Add incomplete and failing test
* migrations/auth/20221117_02_fmuZh-create-group-users-table.py: new migration
* tests/unit/auth/test_migrations_create_tables.py: test new migration
* tests/unit/auth/test_migrations_indexes.py: test new migration
* gn3/auth/authorisation/groups.py: Add `GroupRole` type. Fix typing
  annotations. Fix bugs.
* tests/unit/auth/conftest.py: Fix bugs.
* tests/unit/auth/test_groups.py: Fix test to run.
* gn3/auth/authorisation/groups.py: Assign the group leader at group creation
  time.
* tests/unit/auth/test_groups.py: Ensure the group leader is only ever a
  member of a single group.
The `top_n` argument is used to pick the top `n` results of the computation,
but in this function, it is way too early to do that, since you need to run
filters against the computation results before you pick the results.

* gn3/computations/rust_correlation.py: Remove `top_n` argument
Copy link
Collaborator

@BonfaceKilz BonfaceKilz left a comment

Choose a reason for hiding this comment

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

Nothing seems off to me with this. You could have this merged if you are happy with it.

@zsloan
Copy link
Contributor

zsloan commented Nov 25, 2022 via email

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.

None yet

3 participants