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

Develop #1

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

Develop #1

wants to merge 9 commits into from

Conversation

YiuBen
Copy link
Collaborator

@YiuBen YiuBen commented Aug 3, 2022

No description provided.

@YiuBen YiuBen marked this pull request as ready for review August 5, 2022 09:35
Copy link
Member

@lpatiny lpatiny left a comment

Choose a reason for hiding this comment

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

I could not see the meta information. Are they parsed ?

);
const read = readFile(arrayBuffer);
const image = createImage(read);
/* //uncomment to save files locally
Copy link
Member

Choose a reason for hiding this comment

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

Don't leave commented bloc of code in the code. You can however systematically save the image and this can be part of the test.

I guess to compare images you could use https://www.npmjs.com/package/jest-image-snapshot

Copy link
Member

Choose a reason for hiding this comment

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

You could also add a test that compares part of the object (meta information like width, height but not the data itself).
It could be a to match snapshot but part of the test could also be done directly in the code using https://jestjs.io/docs/expect#tomatchobjectobject

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Metadata is contained in the header attribute of read in this instance.

);
hardScale = parseFloat(zScaleExec.groups.hardScale);
}
// ActualData = hardValue * softScale, hardValue = raw value * hardScale
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the following code because map creates a new array that is not used so this code seems useless (and apparently not tested).

'.map' is also quite slow so better to make a simple loop (and change the data inplace ???)
Strange also that you multiplay hardScale * softScale. Is this correct ? If yes there should be precalculated once for ever if they are constant.

Copy link
Collaborator Author

@YiuBen YiuBen Aug 10, 2022

Choose a reason for hiding this comment

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

It is indeed correct and hardScale * softScale should be constant yes. I will replace map with a loop, thank you for the input.

@lpatiny
Copy link
Member

lpatiny commented Aug 10, 2022

There could be a new property 'metadata' that contains an object with all the meta information.

@targos
Copy link
Member

targos commented Aug 10, 2022

I'm not sure this package should depend on image-js. For the other decoders, we added support inside image-js directly.

package.json Show resolved Hide resolved
@lpatiny
Copy link
Member

lpatiny commented Aug 10, 2022

Indeed image-js should be a dev dependency

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.

3 participants