Skip to content

Clean up factor range and add factor method#14037

Merged
bryevdv merged 15 commits into
branch-3.6from
bv/factor-range-cleanup
Aug 27, 2024
Merged

Clean up factor range and add factor method#14037
bryevdv merged 15 commits into
branch-3.6from
bv/factor-range-cleanup

Conversation

@bryevdv

@bryevdv bryevdv commented Aug 23, 2024

Copy link
Copy Markdown
Member

This is preparation for working on #601. In order to implement that we need and inverse method for synthetic that returns factors, given a synthetic coordinate as input. Looking at adding that, it became clear that factor_range.ts was long overdue for serious cleanup.

This PR breaks up the parts of factor_range.ts in to much smaller functions and methods, adds more tests for these where possible, and also adds a factor method to do the inverse synthetic calculation.

All of the original unit tests were maintained, modulo trivial cosmetic changes for some renames, etc.

I expect map_one_level, etc could also be cleaned up. But since they aren't too terrible I decided to avoid more change risk than necessary.

Edit: examples report checked, everything looks correct at a glance.

Comment thread bokehjs/src/lib/models/ranges/factor_range.ts Outdated
Comment thread bokehjs/src/lib/models/ranges/factor_range.ts Outdated
Comment thread bokehjs/src/lib/models/ranges/factor_range.ts Outdated
Comment thread bokehjs/src/lib/models/ranges/factor_range.ts Outdated
Comment thread bokehjs/src/lib/models/ranges/factor_range.ts Outdated
Comment thread bokehjs/src/lib/models/ranges/factor_range.ts Outdated
@bryevdv

bryevdv commented Aug 23, 2024

Copy link
Copy Markdown
Member Author

@mattpap suddenly seeing this locally (and I guess in CI) after merging latest branch-3.6

Running in Chrome/127.0.6533.120 using devtools protocol 1.3
warning: 127.0.6533.120 is not supported; officially supported version is 118.0.5993.88
internal error: failed to collect tests: ReferenceError: Tests is not defined
    at <anonymous>:1:1
[14:43:33] Finished 'test:unit' after 1.12 s

any ideas?

Edit: I guess it's the last commit for the internal properties but the error message does not give much to go on

Edit2: missed the setv with "levels" at the end of _init

@bryevdv

bryevdv commented Aug 23, 2024

Copy link
Copy Markdown
Member Author

@mattpap next issue.. I am not seeing any eslint issues locally, even though AFAICT I have exactly the same version installed as CI:

dev312 ❯ npm list |grep lint
│ ├── @types/eslint@8.56.7
│ ├── eslint@8.57.0 deduped
│ ├── typescript-eslint@7.16.0
├── eslint@8.57.0

Edit: dunno why the local/CI diff but it's better as a switch in any case

@bryevdv
bryevdv requested a review from mattpap August 23, 2024 23:12
@bryevdv

bryevdv commented Aug 26, 2024

Copy link
Copy Markdown
Member Author

OK I embarked on a (slightly) more ambitious refactoring. It bothered me that we had to switch on the levels in multiple various places, so I encapsulated everything for each level in a subclass of a generic FactorMapping. This also makes it more explicit how it is ok for an L2 mapper to map L1 factors, etc. and also hides all the "boxed" handling purely internally. I still did not alter map_one_level, etc except to tighten up their types, in order to keep the bulk of existing test as-is.

Edit: went through examples report in rather fine detail and everything looks correct there as well.

Comment thread bokehjs/test/unit/models/ranges/factor_range.ts Outdated
Comment thread bokehjs/test/unit/models/ranges/factor_range.ts Outdated
@bryevdv bryevdv mentioned this pull request Aug 26, 2024
5 tasks
@bryevdv
bryevdv requested a review from mattpap August 26, 2024 15:00
Comment thread bokehjs/src/lib/models/ranges/factor_range.ts Outdated
Comment thread bokehjs/src/lib/models/ranges/factor_range.ts Outdated
@bryevdv
bryevdv merged commit c7368dc into branch-3.6 Aug 27, 2024
@bryevdv
bryevdv deleted the bv/factor-range-cleanup branch August 27, 2024 04:25
@github-actions

Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Dec 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants