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

demo/neural-network-demo #21135

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

demo/neural-network-demo #21135

wants to merge 16 commits into from

Conversation

jszuminski
Copy link
Contributor

@jszuminski jszuminski commented May 7, 2024

Added a neural network demo for the demo pages.


Demo

Link to a JSFiddle: https://jsfiddle.net/BlackLabel/3whL2sfe/

Things to consider

We might consider (in the future or now in this demo) adding a panel of inputs where the user could choose the number of layers and the number of nodes for each of the layers (but wouldn't this be an overkill in a simplistic demo that we want to release now?).

@highsoft-bot
Copy link
Collaborator

File size comparison

No differences found

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented May 7, 2024

Visual test results - No difference found


Samples changed

Change type Sample
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Modified samples/highcharts/demo/neural-network/demo.js
Added samples/highcharts/demo/neural-network/demo.js

@jszuminski jszuminski marked this pull request as draft May 7, 2024 15:00
@jszuminski jszuminski requested a review from pawelfus May 7, 2024 15:15
@jszuminski jszuminski marked this pull request as ready for review May 7, 2024 15:15
Copy link
Contributor

@pawelfus pawelfus 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!

Could you check why we see chart skew in mobile view? See:
image

I guess this might be related to computed yAxis extremes.- at least I would start there. Alternatively, can we reduce labels to short names (ReLU, tanh etc)?

samples/highcharts/demo/neural-network/demo.js Outdated Show resolved Hide resolved
samples/highcharts/demo/neural-network/demo.js Outdated Show resolved Hide resolved
@jszuminski
Copy link
Contributor Author

I've added the requested changes. This is an updated demo in JSFiddle: https://jsfiddle.net/47uaj3cs/

Also, I was thinking that maybe instead of layers and activation arrays of numbers, it would be more intuitive for the users to create an array of objects like this:

const layers = [{
  nodes: 6,
  activation: 'tanh'
}, {
  nodes: 6,
  activation: 'ReLU'
}, {
  nodes: 6,
  activation: 'ReLU'
}, {
  nodes: 2,
  activation: 'sigmoid'
}]

What do you think @pawelfus @TorsteinHonsi?

@jszuminski jszuminski requested a review from pawelfus May 16, 2024 07:22
@pawelfus
Copy link
Contributor

Sounds good!

@jszuminski
Copy link
Contributor Author

Here's an updated JSFiddle: https://jsfiddle.net/4p2bLkhw/

Ready for rereview @pawelfus

@pawelfus pawelfus requested a review from marvin19 May 20, 2024 06:52
Copy link
Contributor

@pawelfus pawelfus left a comment

Choose a reason for hiding this comment

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

Thanks!

@pawelfus
Copy link
Contributor

Adding Marita for a11y review

@jszuminski
Copy link
Contributor Author

Most up to date demo: https://jsfiddle.net/ewcxdh5b/

Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi left a comment

Choose a reason for hiding this comment

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

Thanks! A couple of minor change requests inline.

In addition, I notice that the lines are jagged, like anti-aliasing is off. That's because multiple lines are overlaid, right? Would it be an idea to make the series semi-transparent so that it becomes more visible which paths are more frequent?

samples/highcharts/demo/neural-network/demo.js Outdated Show resolved Hide resolved
samples/highcharts/demo/neural-network/demo.details Outdated Show resolved Hide resolved
@jszuminski
Copy link
Contributor Author

@TorsteinHonsi most up to date demo: https://jsfiddle.net/BlackLabel/2a1y5zhe/

I've removed the colors array and added it to plotOptions.line.color, but I've also reduced the opacity of the lines.

In my opinion, it looks better now. What do you think?

Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi 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, thank you!

@jszuminski
Copy link
Contributor Author

As discussed with @marvin19, we are removing point.descriptionFormat temporarily as there are a few problems related to it independent to this pull request.

vendor/moment.js Fixed Show fixed Hide fixed
Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi left a comment

Choose a reason for hiding this comment

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

Thanks!

  1. The descriptionFormat works for me like this
    accessibility: {
        typeDescription: 'Neural network chart',
        point: {
            descriptionFormat:
                'node on {series.xAxis.options.custom.layers.(x).label}'
        }
    },

Is that okay or are there other problems?

  1. The files of the vendor folder are added in this PR. Those should be removed.

@jszuminski
Copy link
Contributor Author

@TorsteinHonsi

  1. Your point.descriptionFormat seems to work as it should, thanks! There were some other problems that @marvin19 found with descriptionFormat, but they are unrelated to this PR so with your change it's good to go.

  2. Sorry, I completely overlooked that, don't know where this came from.

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

5 participants