Filling a buffer inside the parameters

I have this piece of code in my DLL:
void myFunc(double bufferToFill[3])
{
  bufferToFill[0] = -0.5;
  bufferToFill[1] = 7.5;
  bufferToFill[2] = 3;
}

I want to fill the bufferToFill buffer from the DLL to my NodeJS app.
I use the following, NodeJS code which is not working.
var toFill = new Array(3).fill(0)

const res = load({
      library: "myDLL",
      funcName: "myFunc",
      retType: DataType.Void,

      paramsType: [DataType.DoubleArray],
      paramsValue: [bufferToFill],
})

The output is just a bunch of 0 (zero).
Any idea on how to make this work?

where you print the output?

extern "C" void bufferToFill(double bufferToFill[3]) {
  bufferToFill[0] = -0.5;
  bufferToFill[1] = 7.5;
  bufferToFill[2] = 3;
  printf("%f", bufferToFill[0]);
}

My output is -0.500000

Yes, inside the DLL it is correct, but inside the NodeJS app, it's incorrect.
var bufferToFill= new Array(3).fill(0)

const res = load({
      library: "myDLL",
      funcName: "myFunc",
      retType: DataType.Void,

      paramsType: [DataType.DoubleArray],
      paramsValue: [bufferToFill],
})

console.log(bufferToFill) //prints: [0,0,0]

you can not modify js valuememory in c libary directly，you should return a new array which is better
发自我的iPhone
------------------ Original ------------------
From: aidv @.>
Date: Tue,Oct 17,2023 10:01 PM
To: zhangyuang/node-ffi-rs @.>
Cc: yuuang @.>, Comment @.>
Subject: Re: [zhangyuang/node-ffi-rs] Filling a buffer inside the parameters(Issue #7)
Yes, inside the DLL it is correct, but inside the NodeJS app, it's incorrect.
var bufferToFill= new Array(3).fill(0) const res = load({       library: "myDLL",       funcName: "myFunc",       retType: DataType.Void,       paramsType: [DataType.DoubleArray],       paramsValue: [bufferToFill], }) console.log(bufferToFill) //prints: [0,0,0]
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.Message ID: @.***>

Ok thanks.
Is it possible to add this feature?

i will try it later，but it doesn‘t always succeed，and it's not a safe operation
发自我的iPhone
------------------ Original ------------------
From: aidv @.>
Date: Tue,Oct 17,2023 10:28 PM
To: zhangyuang/node-ffi-rs @.>
Cc: yuuang @.>, Comment @.>
Subject: Re: [zhangyuang/node-ffi-rs] Filling a buffer inside the parameters(Issue #7)
Ok thanks.
Is it possible to add this feature?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.Message ID: @.***>

Thanks. It's worth trying maybe.

I need this feature too. Several libraries I use were built this way. For example, libraries where its return is an integer representing the execution status and receiving two pointers, one a string and the other an integer.

In the latest version, ffi-rs support modify buffer data in place like
extern int modifyData(char* buffer) {
    // modify buffer data in place
}

const arr = Buffer.alloc(200) // create buffer
const res = load({
  library: "libsum",
  funcName: "modifyData",
  retType: DataType.I32,
  paramsType: [
    DataType.U8Array
  ],
  paramsValue: [arr]
})
console.log(arr) // buffer data can be updated


Add network name to metadata

See:

https://github.com/helium/helium-packet-router/issues/285
https://github.com/helium/helium-packet-router/pull/286

In the metadata for a gateway that is sent to ChirpStack, explicitly state that this packet comes from the ThingsIX network. This would help services on the other side of ChirpStack know for sure if the packet was received by ThingsIX, without having to guess from the other metadata fields.

 Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

Thank you for the contribution @jpmeijers! I'm happy to merge this PR.
I'm out of office until early January. I will release a new version with this change in January.


feat: Add metadata fields to core entities

Thanks for submitting a PR! Please check the boxes below:

[X] I have run pre-commit to check linting
[x] I have added information to docs/ if required so people know about the feature!
[x] I have filled in the "Changes" section below?
[x] I have filled in the "How did you test this code" section below?
[x] I have used a Conventional Commit title for this Pull Request

Changes
PR Re-created (ORIGINAL: https://github.com/Flagsmith/flagsmith/pull/2861)
FE changes:

Add and create metadata to Environments, segments, and Flags
The feature was wrapped inside the 'enable_metadata' flag
BE changes:
Support metadata for segments, and Flags
Support required metadata model fields for segments, and Flags

How did you test this code?

Go to manage
Click the Metadata tab.
Create Metadata.
Edit metadata and enable any entity

Codecov Report
Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (f770ff1) 95.99% compared to head (6cbd20c) 95.99%.
Report is 5 commits behind head on main.

Files
Patch %
Lines

api/features/serializers.py
81.25%
3 Missing :warning:

api/segments/serializers.py
84.61%
2 Missing :warning:

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3212      +/-   ##
==========================================
- Coverage   95.99%   95.99%   -0.01%
==========================================
  Files        1062     1062
  Lines       32265    32348      +83
==========================================
+ Hits        30973    31051      +78
- Misses       1292     1297       +5

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

It's not obvious what's going on here:

What is metadata?
Where do I create it?

This should have a title

UX of this needs improving, maybe a dropdown of related entities, clicking adds item to table with columns entity, required
This should be viewable when creating metadata

Think this could be improved

Instead of fixed columns for feature etc, we could have  2 columns : Name and Entity with a standard table filter for entity. The entities could then be the standard chip component, perhaps colour denoting required

This should have a title

UX of this needs improving, maybe a dropdown of related entities, clicking adds item to table with columns entity, required
This should be viewable when creating metadata

This is the new proposal:

It's not obvious what's going on here:

What is metadata?
Where do I create it?

I think the no results message could just say your project has no metadata items. also should be centered and given more padding

Here is the new proposal for adding metadata:

Think this could be improved

Instead of fixed columns for feature etc, we could have  2 columns : Name and Entity with a standard table filter for entity. The entities could then be the standard chip component, perhaps colour denoting required

(Rough example)

Here is the new proposal for Metadata fields row:

It's not obvious what's going on here:

What is metadata?
Where do I create it?

I think the no results message could just say your project has no metadata items. also should be centered and given more padding

Here is the new proposal for adding metadata:

I think for this to be intuitive, it should auto populate required fields here. I assume at the moment it requires you to click the dropdown and add each metadata item that's required?

It's not obvious what's going on here:
Here is the new proposal for Metadata fields row:

Makes sense, though it has the default purple border and I think it might make sense to add a * at the end

It's not obvious what's going on here:
Here is the new proposal for Metadata fields row:

Makes sense, though it has the default purple border and I think it might make sense to add a * at the end

Change the default purple border to red, and add * to Required entities

It's not obvious what's going on here:

What is metadata?
Where do I create it?

I think the no results message could just say your project has no metadata items. also should be centered and given more padding

Here is the new proposal for adding metadata:

I think for this to be intuitive, it should auto populate required fields here. I assume at the moment it requires you to click the dropdown and add each metadata item that's required?
Also the delete header should be inline with the action button.

This is the new proposal for adding metadata


Update method hints of Route Facade

Update method hints to reflect return type of Illuminate\Routing\Router

add hints methods prefix(), middleware(), name() ...

Hi @rafaelbeckel,
I've added something similar to my RouteRegistrar helper class: https://github.com/ARCANEDEV/Support/blob/master/src/Routing/RouteRegistrar.php#L9-L35
This may helps you to add more methods to the facade.


[Don't merge] Nikita/Affiliate signup flow

Changes:

...

Type of change

[ ] Bug fix
[x] New feature
[ ] Update feature
[ ] Refactor code
[ ] Translation to code
[ ] Translation to crowdin
[ ] Script configuration
[ ] Improve performance
[ ] Style only
[ ] Dependency update
[ ] Documentation update
[ ] Release

hello, @nikita-binary if a PR is not ready for review, you can draft it instead of adding [Don't Merge] on the PR title.


What is _spPageContextInfo

Category

[x ] Question

Referring to your code https://docs.microsoft.com/en-us/sharepoint/dev/sp-add-ins/upload-a-file-by-using-the-rest-api-and-jquery#RunTheExamples
Specifically Code 2. function uploadFile()  Line :
    // Get the server URL.
  var serverUrl = _spPageContextInfo.webAbsoluteUrl;
Where have you declare _spPAgeContextInfo?

That is a global (legacy) property existing on all pages in classic SharePoint mode.

Thank you, however, I am creating a web component (ascx), is there any way I can this example? Do we have to change ascx to SharePoint mode? how?

@RupenAnjaria how do you intend to use the User Control (ascx) that you're building? Will you embed it in a web part or put it directly on a Master Page or a Page Layout? Either way, the _spPageContextInfo should be already available to you as @wictorwilen mentioned.

I will be using the control in Page Layout.

Just to be clear, it's a web application (so I have to refer code Sample 1), right?

The hostWebUrl will be https://myCompany.sharepoint.com/MyApplication/


Object from imported package missing Validate() method

Problem statement
In the swagger specification, I used an object from the 3rd party package.
ThirdPartyObject:
    type: object
    x-go-type:
         import:
             package: "ThirdPartyPackage/v1"
         type: "ThirdPartyObject"

This object is used in a post request.  The generated swagger code assumes the object has an Validate() method, however the ThirdPartyObject has no Validate() method.
if runtime.HasBody(r) {
		defer r.Body.Close()
		var body v1.ThirdPartyObject
		if err := route.Consumer.Consume(r.Body, &body); err != nil {
			if err == io.EOF {
				res = append(res, errors.Required("body", "body"))
			} else {
				res = append(res, errors.NewParseError("body", "body", "", err))
			}
		} else {
			// validate body object
			if err := body.Validate(route.Formats); err != nil {
				res = append(res, err)
			}

Any workaround for this issue?

Is this for a client or a server?

@casualjim For a server which accepts post request.
If you define the post api which requires the ThirdPartyObject as the request body, the generated code would use the method ThirdPartyObject.Validate()

@casualjim BTW, can I let go-swagger skip generating Validate() method for the request body?
var body v1.ThirdPartyObject
if err := body.Validate(route.Formats); err != nil {
				res = append(res, err)
}

@YanzhaoLi  go-swagger expects all models to implement the github.com/go-openapi/runtime.Validatable interface.
You can always embed your third-party object in your own definition like shown below and refer to that redefined type in your spec.
type MyThirdPartyObject struct {
  ThirdPartyObject
}

func (m MyThirdPartyObject)  Validate(_ strfmt.Registry) error {
  return nil
}


Test that node input is not all zeros

I've added a test to make sure that the node input is not all zeros when copying off the board.
@mundya @tcstewar: This is the problem I was getting yesterday. It came up again today, even using my new IP settings. I realized yesterday that @tcstewar's original suggested IP settings should have worked (this is basically what I put in the README, I think), but for some reason they didn't. Though they did seem to work when I went back to them after getting the board working with the other settings. All very strange.
Anyway, this test should at least let users know if this is happening. It's kind of slow, though, I guess because it spools up a simulator? Maybe it can be re-written to more specifically test the problem.
We'll have to spend more time at some point trying to figure out why this occurs, and whether we can have better IP suggestions to avoid it.

Hmm...  when it came up again today, did it continue to be a problem even when the model was run for a long time (20+ seconds)?
We have sometimes seen this happen briefly at the beginning of a run, but goes away after a ten seconds.
I'd thought the problem that we fixed before was due to the gateway setting, which should have been left blank, but you'd said you set to the same IP address as the board.

I think you gave me some settings first, that had the gateway empty. It was when I used Ian's settings that I had the gateway set.
I didn't run it for a long time, so maybe it was just due to run length. But when I changed the IP, it seemed to work immediately, which suggests it's IP related. I'll play around more.

Sorry for the huge delay in getting back to you on this. Would you be happy for me to make this code a utility that a user can be expected to call rather than have it sit in the tests?

Sure, that could work.


Document actual release process against aws' repo

We used to open heptio:master against aws:develop. The reason for this got lost in the shuffle and then we started opening PRs against aws:master directly.
We should figure out and document what the actual release process is for this.

@liztio has done this last cycle.


ENH: Read eyetracking data (Eyelink) (Fork of #10855 )

refactored code initially pushed in PR #10855 by @dominikwelke and reviewed by @larsoner and @drammock. Starting a new PR as suggested by @larsoner
briefly: When plotting the test_eyelink.asc file, The eye channel traces now appear at the beginning of the time series as expected.  I  also added a dataframes attribute to the RawEyelink class, which returns the pandas dataframes that are created while loading the file.
@dominikwelke I'll leave some comments on the code in your PR to explain some of the changes I made in the code refactoring, and you can let me know if you agree, and we can discuss what needs to be worked on next!
@sportnoah14 also cc'ing you here so that you can stay in the loop.

@scott-huberty looks like this needs a rebase or a merge-main.  If it were me, I'd probably
git rebase -i 4a390cc^

and then change pick to s (or squash) for all the commits @dominikwelke did.  That way you're only applying 3 commits on top of current main instead of 27 so resolving the merge conflicts might go much more smoothly.

git rebase -i 4a390cc^

Thanks Dan - I think I did that correctly.. I had to force push because my local branch was behind remote after rebasing.

I think I did that correctly.. I had to force push because my local branch was behind remote after rebasing.

Force-push is expected here (you have now 2 commits where GitHub had 27), and the monster multiline commit message for 5efcf53 suggests that you did it correctly... except that this was only the first step.  Now you have to fetch upstream main and then git rebase upstream/main, to then actually fix the merge conflicts.

I think I did that correctly.. I had to force push because my local branch was behind remote after rebasing.

Force-push is expected here (you have now 2 commits where GitHub had 27), and the monster multiline commit message for 5efcf53 suggests that you did it correctly... except that this was only the first step. Now you have to fetch upstream main and then git rebase upstream/main, to then actually fix the merge conflicts.

Sorry for the delay - While doing git rebase upstream/main I needed to manually resolve conflicts that couldn't be merged, it's a new process for me but I hopefully did this correctly.

hi @scott-huberty
i think ill finally find some time to work on this again, while working on pupil-size data..
what is the status on your branch?
i saw you did quite some reorganisation of the eyelink parser code.. generally ok with me, i am not bound to my initial layout
yet, maybe worksplitting makes sense to avoid double structures?
i am currently working with data that has already been synchronized with EEG, so i can focus on basic integration (e.g. the ch_types FIFF stuff) and (pre)processing after having it loaded, while you further work on the parser?
what do you think?

@dominikwelke I think this is a great idea!
I should wrap up the code for the parser next week. Then I can start advancing the test-suite that you starting.
I can also create a new local branch and cherry-pick your recent commits to make sure the parser code remains compatible with the development on channel types etc you are doing right now.
I'll follow up with you next week when the parser code is finalized, and maybe we can start to merge our work then.

Hey @dominikwelke , I think the parser is looking good!

I haven't really touched your code on setting channel and FIFF types, but I see you did some refactoring of that last week. So I will try to pull your recent commits to a copy of this branch.
On that note, this Parser should be robust to reading all the various eyelink channel types that can be  present in an eyelink file (Gaze, Pupil, Resolution, Velocity, Head target (remote mode), and DINS). But, I'm not sure what coils/units etc we should give these channels, what do you think?
I've started a github repo that contains various eyelink files that I've been using to test code during development, including monocular files and files with resolution/head target/DIN channels. Feel free to check it out.

One comment on the DIN (digital input port) data in Eyelink files.. Eyelink tracks the pin downs/ups of a DIN port for every sample of the file, so if I'm not mistaken, I think that we could load this data in as a stim channel. In my lab, we are sending info to the eyelink host PC's DIN port, and get values between integer values between 0-3 in our files (0 being pin down, and 1,2, or 3 depending on what pin is up).

One comment on the DIN (digital input port) data in Eyelink files.. Eyelink tracks the pin downs/ups of a DIN port for every sample of the file, so if I'm not mistaken, I think that we could load this data in as a stim channel.

Following up on that, if we read in the DIN data as a stim channel, this is an example of what it would look like.
for this example file, a photodiode was connected to Eyelinks DIN port. This is a pupillary light reflex task, so a white flash on the screen triggers the photodiode (and thus a change in the pins of the DIN port). This is why, in this file, just after the DIN value changes at around 2-seconds, you see the pupil size constrict (in the pupil channel).

nice, great progress @scott-huberty !
i'll test it with some of my files soon and give feedback.
immediate comments:

my initial parser function was based on an already existing project.
if you kept any of it, we should acknowledge the author (and the copyright note, as it's MIT licensed)

this Parser should be robust to reading all the various eyelink channel types that can be present in an eyelink file (Gaze, Pupil, Resolution, Velocity, Head target (remote mode), and DINS). But, I'm not sure what coils/units etc we should give these channels, what do you think?

treating DINS as stim channel makes sense, imo.
for the other channel types:
we can define new coil types, if needed.
but what do you think is the priority of it? do ppl actually use these data?
i think setting specific channel info makes sense IF there will be methods for these data types. otherwise we can simply give them the misc channel type and leave it to the users to treat the data as wanted.
an alternative/addition could be a not implemented warning

how should we proceed? (also @drammock @larsoner )
if you want @scott-huberty - and your work on the parser is more or less done - i can deal with the integration of our branches.
i would do so by merging your branch into mine, as the associated PR has had a bit more public discussion than yours here.
of course your branch can stay active and news can be merged again down the line

treating DINS as stim channel makes sense, imo.

Agreed

for the other channel types: we can define new coil types, if needed. [...] otherwise we can simply give them the misc channel type

misc makes sense to me unless there is a compelling reason to add a more specific channel type

Ok, let's start with misc, I can look at some test files after to make sure everything looks okay.

my initial parser function was based on an already existing project.
if you kept any of it, we should acknowledge the author (and the copyright note, as it's MIT licensed)

The code from that project isn't in this branch anymore. The code in this branch I developed along with @christian-oreilly , (who I've listed as a 3rd contributor in the eyelink.py file). The code you wrote for initializing the RawEyelink class, creating the channel/coil types, starting the test-suite etc. is in this branch too.

how should we proceed? (also @drammock @larsoner )
if you want @scott-huberty - and your work on the parser is more or less done - i can deal with the integration of our branches.
i would do so by merging your branch into mine, as the associated PR has had a bit more public discussion than yours here.
of course your branch can stay active and news can be merged again down the line

What do you think about the idea of finishing up this PR (basically wrapping up setting the channel types as discussed, and completing the test-suite and docs), which won't include the preprocessing functions that you've been working on in your branch. Then, assuming the PR is approved, you could merge main from your branch and carry forward the preprocessing functions in a second PR (the one currently open). IMO, this might create a nice "separation of concerns" as far as testing, documenting, and reviewing read_raw_eyelink and the additional preprocessing functions. If so, you could pull this branch now and make your own commits to it too, which we could merge into the PR.
Alternatively, yes we could merge this into your branch, and continue working on it from there. In that case, there's a small amount of refactoring I wanted to do, but I can do it before or after the merge. just let me know what you're thoughts are! (Others please feel free to weigh in on this too if you don't like my proposed idea).

What do you think about the idea of finishing up this PR (basically wrapping up setting the channel types as discussed, and completing the test-suite and docs) [... and then] carry forward the preprocessing functions in a second PR

I have not read all previous responses, but I will say from a reviewer perspective ideally the I/O and preprocessing could go in separate, sequential PRs. It will make reviewing much easier for us!

Concerning the choice of branch/PR: moving the preprocessing code from my current branch into a separate branch+pr would be a matter of seconds, as it happens in entriely different files. also no real discussion of this code has happened in the PR so far.
my further arguments for merging into my branch were:  [...but ] we've discussed this long enough - you have a preference yor your branch and the reviewers dont seem to care so lets take your branch :)

It's up to you - Either way, I'm just glad we've progressed to the stage that we can start working on a common branch!

so please, no changing of the git history in your branch from now on.

Perhaps I made a faulty assumption that no one had fetched this remote branch - of course, we'll keep the history linear from now on!

i tested with some of my files and everything seems to work swiftly!

Awesome!

Thanks for figuring out a plan and workflow @scott-huberty @dominikwelke -- feel free to clean up / modify this PR as necessary then ping me in a comment for review!
From a very quick look at the PR, it looks like there is no new entry in tutorials/io or `tutorials/preprocessing' -- maybe add one to the latter that just shows you can read some data, epoch it, and make an average of some sort (e.g., of pupil size)? Even if really messy because the preprocessing functions are missing it's a good place to see that at least some things are working as expected. Then we can build the tutorial up as standard methods are added

... probably not terribly useful, but in case you want some inspirition here is a tutorial I made ages ago for the defunct pyeparse
https://github.com/pyeparse/pyeparse/blob/master/examples/plot_from_raw_to_epochs.py

maybe add one to the latter that just shows you can read some data, epoch it, and make an average of some sort (e.g., of pupil size)? Even if really messy because the preprocessing functions are missing it's a good place to see that at least some things are working as expected. Then we can build the tutorial up as standard methods are added

Thanks @larsoner ! Will do. I also saw some pydocstyle/docstring errors too, I'll look at them tomorrow.
@dominikwelke I merged your PR on the work you did with the FIFF types - I'm going to give it a closer look tomorrow and may have some suggestions if that's okay with you.
FYI I think you have push privileges on this branch now. I'm okay with you pushing directly here, I think we will be able to manage : )

Thanks again @dominikwelke for merging in your updates - I can tell that you put a lot of work into this!
As I understand it, the code follows these steps:

Create a list of channel names
Create a list of channel types
Create an mne.Info object, (passing in the channel names, sfreq, channel types)
Iterate over each dict in info['chs'], and specify the coil_types, units etc.

Thanks again @dominikwelke for merging in your updates - I can tell that you put a lot of work into this!
As I understand it, to create the info object and set the channel types/coil types/units etc, the code is broken down into these steps:
1. Create a list of channel names
2.  Create a list of channel types
3. Create an mne.Info object by passing in the channel names, sfreq, channel types) info mne.create_info
4. Iterate over each dict in info['chs'], and specify the coil_types, units etc.
I find this to be a bit complicated because I think some mistakes are being made in step 2 and step 3 and so we are spending a lot of time in step 4 correcting things..

In step 2, all the channels are given the type 'eyetrack' , even if it is not an eyetracking channel (the 'DIN' channel should be a stim type):

https://github.com/mne-tools/mne-python/blob/b5ca7f97c968486ca7df059308bcf9f93d06044d/mne/io/eyetracking/eyelink.py#L590

Even for the channels that are rightly 'eyetrack' channels, in step 3 mne.create_info does not correctly set the 'unit', 'coil_type, and 'loc' keys in the info['chs'] dict to what we want them to be.

Take the example above. From what I understand of the code, we actually want

Pupil channels to have a 'coil_type' of FIFF.FIFFV_COIL_EYETRACK_PUPIL and a unit of FIFF.FIFF_UNIT_MM,

xpos/ypos channels we want a 'coil_type' of FIFF.FIFFV_COIL_EYETRACK_POS and a unit of  FIFF.FIFF_UNIT_PX

Thus in step 4, we spend this whole block of code iterating over the info['chs'] dict, correcting the values that weren't set correctly before:
https://github.com/mne-tools/mne-python/blob/b5ca7f97c968486ca7df059308bcf9f93d06044d/mne/io/eyetracking/utils.py#L117-L200

Can't we amend mne.io.pick.get_constants, to split  eyetrack into two types, (something like eyetracking_pupil, eyetracking_pos), setting the coil_types, units etc. accordingly, so that mne.create_info,  will return the info correctly? : ) It looks like this is what is done for fnirs channels, so we could follow a similar approach.
https://github.com/mne-tools/mne-python/blob/b5ca7f97c968486ca7df059308bcf9f93d06044d/mne/io/pick.py#L18-L104
I think this would make things much simpler for us!!!
Thanks again for starting all that work! I'm sure it was a big undertaking.

FYI I think you have push privileges on this branch now. I'm okay with you pushing directly here, I think we will be able to manage : )

yup, i accepted the invitation but my pushs were still rejected. hope i can solve this for subsequent commits.

find this to be a bit complicated ... we are spending a lot of time in step 4 correcting things

i totally agree! the complexity here comes from the necessity to chose between different channel/coil types (position/pupil/...) based on suboptimal information (channel name).
..and there are different possible units  for the coil types:
px, deg, mm(?) for position, and au or mm for pupil
but thats less of a problem..

Can't we amend mne.io.pick.get_constants, to split eyetrack into two types, (something like eyetracking_pupil, eyetracking_pos)

yes, this would be my suggestion to reduce the complexity too. and this actually bothered me for a while :)
i didnt do that in the initial design because of the drafting/discussions during the code-sprint and (of course) because i wasnt fully aware of the consequences and the details of the fiff handling. then it stuck.
yet it makes total sense to have different channel_types for different channel types and separating them on the highest implementation level will also be useful as preprocessing and analysis methods will (at least partly) differ.
i will change this today, its not much work

yup, i accepted the invitation but my pushs were still rejected. hope i can solve this for subsequent commits [...] i will change this today, its not much work

I think the issue was on my end. I changed this branch's settings so let me know if you still have the issue when you try to push your commits today.

..and there are different possible units for the coil types:
px, deg, mm(?) for position, and au or mm for pupil
but thats less of a problem..

As I understand, mne.io.pick.get_constants typically sets a single kind, coil_type, and unit based off the channel_type, so we should probably choose the coil_type/unit that are most commonly reported for the channel_type across systems (eyelink/Tobii). I think that would be pixels for eye-position and arbitary units for Pupil.

yet it makes total sense to have different channel_types for different channel types and separating them on the highest implementation level

Agreed! eye-position and pupil data are not on the same scale, and they are commonly reported across eyetracking systems (so If that Tobii I/O ever gets picked back up, they would likely utilize the pupil/pos channel_type's too), so it makes sense to me to have two eyetracking channel types. As for the other channels that eyelink can output (velocity etc.), we can just set the channel_type to misc as discussed above.
Thanks!

pushing worked now, thanks!
i now implemented it as discussed, and yes - defaults can be defined and i set them to be px for eyetrack_pos and mm for eyetrack_pupil (though i guess most eyetrackers probably spit out AU - anyway, that's minor)
out of the box the reader now guesses eye and x/y axis (and
unit, but thats usually not there) from the ch_name and puts NaN otherwise. this should be fine for the start and most cases, but i kept the more flexible setting function in utils because it might be needed down the line (e.g. for anaylses requiring binocular recordings)
read_raw_eyelink gives misc type to everything it doesnt recognize as pos/pupil/stim.
some other reader functions (e.g. the eeg ones) have input arguments to specify channels with certain types (misc/eog...)
we could include smth like this here as well, though i am not sure it is too important. what do you think?

some other reader functions (e.g. the eeg ones) have input arguments to specify channels with certain types (misc/eog...)
we could include smth like this here as well, though i am not sure it is too important. what do you think?

If you mean that this could allow a user to set a channel that currently will be be set as a misc type (velocity, head-distance channel from an eyelink file), to the eyetrack type, I suppose we could do that, if the scalings of those channels are comparable with the defaults in eyetrack? I also don't have a strong opinion about it at the moment.

I also don't have a strong opinion about it at the moment.

ok, then lets not do it for now. if the reviewers think it makes sense, it can be added later.

... probably not terribly useful, but in case you want some inspirition here is a tutorial I made ages ago for the defunct pyeparse
https://github.com/pyeparse/pyeparse/blob/master/examples/plot_from_raw_to_epochs.py

Also @dominikwelke do you have any interest in spearheading the sphinx documentation for this PR (per @larsoner 's suggestion a few threads above)? If I'm not mistaken you've done more work on docs with MNE than I have.
If so, I can start working on the pytest suite for this PR while you work on that. Of course if you don't have time to work on the docs feel free to let me know too.

Also @dominikwelke do you have any interest in spearheading the sphinx documentation for this PR (per @larsoner 's suggestion a few threads above)? If I'm not mistaken you've done more work on docs with MNE than I have.

alright, i'll start with that.

looks like your renaming removed all the git history of the files (deleting file -> creating new file with all new code without history)
if you use git mv instead of renaming in the file explorer the history is kept (see e.g. https://stackoverflow.com/questions/2641146/handling-file-renames-in-git)

looks like your renaming removed all the git history of the files (deleting file -> creating new file with all new code without history)
if you use git mv instead of renaming in the file explorer the history is kept (see e.g. https://stackoverflow.com/questions/2641146/handling-file-renames-in-git)

Sorry just saw this..  Thanks for that tip - I didn't know that!

We will squash+merge when merging anyway, which will destroy any history of mv or whatever. In the end all lines will look like they were committed by a single person (@scott-huberty as author of the PR) with co-author credit to @dominikwelke . So I wouldn't bother with rolling back and doing git mv unless it's important for you to have git blame to work on your branch for the next few days / weeks / whatever until we merge...

We will squash+merge when merging anyway, which will destroy any history of mv or whatever. In the end all lines will look like they were committed by a single person (@scott-huberty as author of the PR) with co-author credit to @dominikwelke . So I wouldn't bother with rolling back and doing git mv unless it's important for you to have git blame to work on your branch for the next few days / weeks / whatever until we merge...

Ok, thanks for clarifying @larsoner
@dominikwelke is that okay with you?

I'm starting to comb through the failing tests:
since we split the eyetrack channel type to eyetrack_pos and eyetrack_pupil , mne/io/array/tests/test_array.py::test_array_raw  fails, I believe because of _split_picks_by_type:
https://github.com/mne-tools/mne-python/blob/8dfb23be27536d654838377622a898b08f86cd31/mne/viz/_mpl_figure.py#L2119-L2145
I think we need to import _EYETRACK_CH_TYPES_SPLIT at the top of mne.viz._mpl_figure.py , and then after line 2133, add
elif ch_type in _EYETRACK_CH_TYPES_SPLIT:
    pick_kwargs['eyetrack'] = ch_type

we  also need to remove eyetrack from this for loop in mne.io.pick , since it no longer must be boolean - it could be 'eyetrack_pos' or 'eyetrack_pupil'
https://github.com/scott-huberty/mne-python/blob/337f63185a1ee93449d6c99b6953bd19d5e4913a/mne/io/pick.py#L433-L439

I think we need to import _EYETRACK_CH_TYPES_SPLIT at the top of mne.viz._mpl_figure.py , and then after line 2133, add (...)

we also need to remove eyetrack from this for loop in mne.io.pick , since it no longer must be boolean - it could be 'eyetrack_pos' or 'eyetrack_pupil'

yes, sounds correct. i only tested it with boolean argument and this worked as intended (picks both pupil and pos channels)

I think we need to import _EYETRACK_CH_TYPES_SPLIT at the top of mne.viz._mpl_figure.py , and then after line 2133, add (...)

we also need to remove eyetrack from this for loop in mne.io.pick , since it no longer must be boolean - it could be 'eyetrack_pos' or 'eyetrack_pupil'

yes, sounds correct. i only tested it with boolean argument and this worked as intended (picks both pupil and pos channels)

OK, I'll make the aforementioned changes and push.

hey @dominikwelke whenever you have time do you mind reviewing the change I made to mne/defaults.py in commit https://github.com/mne-tools/mne-python/pull/11152/commits/02ada5a2806ab8e25d49ce82027df6cbe5ea3db3 . Basically just split the eyetrack scalings from eyetrack=1 -> eyetrack_pos=1., eyetrack_pupil=1.

hey @dominikwelke whenever you have time do you mind reviewing the change I made to mne/defaults.py in commit 02ada5a . Basically just split the eyetrack scalings from eyetrack=1 -> eyetrack_pos=1., eyetrack_pupil=1.

yes, the plot scalings seem to work for both ch_types.
we could also set the default unit for eyetrack_pos to px - btw, i just realised that changing the unit in info['chs'] doesnt affect the depicted unit in plots (it stays with the default).. do you know if this is expected?

For plotting the unit isn't pulled from info but instead somewhere in defaults.py where each channel type has a standard unit. In principle it could/should use info instead but that would require a lot of work I think

Hi @larsoner -
mne.tests.test_tes_cov :: test_regularization is failing because there isn't an argument for the eyetrack channels (i.e. eyetrack=True) in mne.cov.regularize.
https://github.com/mne-tools/mne-python/blob/8dfb23be27536d654838377622a898b08f86cd31/mne/tests/test_cov.py#L458-L472
In that last assert statement above, eyetrack_pos and eyetrack_pupil are in _DATA_CH_TYPES_SPLIT but not in the signature for mne.cov.regularize.
However, in mne, computing the covariance matrix is done for source reconstruction, and eyetracking signals do not contribute to source reconstruction. So I am not sure if we should add a parameter for eyetrack channels in mne.cov.regularize .. Maybe the test should be revised to exclude eyetrack channels from the assert statement?
What do you think?

Cov is used for other stuff, too, like ML algorithms. For best consistency anything that we consider a data channel should be in there.

Ok yeah, I see your point there.

Data usually means "brain data" IIRC though, and stuff like EOG is not included in that definition. It makes me think that pupil data should not be in data ch types lists actually... And then it wouldn't be needed in cov code either

Sure, I can take a shot at removing pupil data from data ch_type lists - but just so I understand :slightly_smiling_face: I'm a bit confused as to why pupil data would be considered any less brain and more akin to EOG than eyepos data? esp since eyepos data reflects eye movements (i.e EOG)?

I think both eyepos and pupil (and EMG, EOG, etc.) are non-data according to MNE def
https://mne.tools/dev/glossary.html#term-data-channels

I think both eyepos and pupil (and EMG, EOG, etc.) are non-data according to MNE def
https://mne.tools/dev/glossary.html#term-data-channels

ohhh ok - thanks for sharing. Yeah that makes sense to me given the definition at that link.
So then neither eyetrack_pos nor eyetrack_pupil should be in data_ch_type lists. In which case they aren't needed in cov code either (as you said before).
I look into removing eyetrack channel types from the data_ch_type lists.

hi @scott-huberty
i started the documentation page and we need some data for it..
is it ok for you if i link to one of the files in your github repo:

use the file you already added

hm, no i dont think that's good data to show because there is no meaningful event information in this file.
id at least want some fixation task, or stimulus onsets to plot a few averaged trials.
besides, no other tutorial or example is using testing data, and it's not accessible for the doc build right now. if i adapt this anyway i can pick a better dataset

maybe we can see about putting the file you use on a more official channel

If the dataset is small (like < 50 MB?) then you could make a PR to mne-misc-data
https://github.com/mne-tools/mne-misc-data
If it's more than this we can make a new mne.datasets.pupil_something.data_path() and host it on MNE's osf.io account

If the dataset is small (like < 50 MB?) then you could make a PR to mne-misc-data

thanks for the suggestion @larsoner but i had already uploaded it to osf

hi @scott-huberty
here an initial tutorial and other doc changes.
hope this doesnt mess with your test work!

hi @scott-huberty
here an initial tutorial and other doc changes. hope this doesnt mess with your test work!

So awesome, thanks @dominikwelke !!

hope this doesnt mess with your test work!

There are still test failures to fix so I'll be going through them anyways. So no big deal to me if a couple of things are added to that list

this last commit would also be a question to the reviewers @larsoner @drammock
recap:
we have some specific channel info for the eyetracker channels that can vary - unit (px/deg, AU/mm), eye (l/r), gaze component (x/y).
these will become relevant with more preprocessing/processing functions (e.g. unit converter, binocular analyses, plots...).
but we require user input to set them (not strictly for this PR though, as the eyelink files contain this info).
acchieving this with the existing set_channel_types function seemed suboptimal to me, as it would make this messier.
on the other hand, the way i wrote it now is an api change.
what do you think, is it good like this or should i rather squeeze it into set_channel_types ?
the def could be changed to accept list/tuple with all info instead of str for eyetrack channels..

this last comm

Thanks @dominikwelke , as a note (after you come to a conclusion with the reviewers) - if set_channel_types stays, can you run
 pytest .\mne\tests\test_docstring_parameters.py

(make sure you have numpydoc installed) - it will raise some style fixes needed in the set_channel_types code

Hey @dominikwelke There's a new test failure from a change in 6ad5b69 . Can you look into why it doesn't like the new unit when you have the time?
________________________________ test_si_units _________________________________
mne/tests/test_defaults.py:41: in test_si_units
    want_scale = _get_scaling(key, units[key])
mne/io/base.py:2123: in _get_scaling
    f'{target_unit} is not a valid unit for {ch_type}, use a '
E   ValueError: px is not a valid unit for eyetrack_pos, use a sub-multiple of AU instead.

what do you think, is it good like this or should i rather squeeze it into set_channel_types ?

I did not look at the current implementation, but I would expect a dedicated eyetracking function for this. There isn't really a use case for changing channel types for other data -- unless you count rare stuff like remapping MEG channels from one system or MEG channel type to another, and we would definitely not do this with set_channel_types.
For fNIRS there are similar transitions, and these live in mne.preprocessing.* and mne.preprocessing.fnirs.*. I think it would make sense to add such eyetracking-specific stuff to a new mne.preprocessing.eyetracking.* namespace. For example, for unit conversions like pixels to degrees, something like mne.preprocessing.eyetracking.convert_eye_units(to, screen_size) or something would make sense.

Hey @dominikwelke There's a new test failure from a change in 6ad5b69 . Can you look into why it doesn't like the new unit when you have the time?
________________________________ test_si_units _________________________________
mne/tests/test_defaults.py:41: in test_si_units
    want_scale = _get_scaling(key, units[key])
mne/io/base.py:2123: in _get_scaling
    f'{target_unit} is not a valid unit for {ch_type}, use a '
E   ValueError: px is not a valid unit for eyetrack_pos, use a sub-multiple of AU instead.

well, indeed px are no SI unit :)
if the units in any plot dont reflect changes in info anyway, i'd pick my battles and just switch back to AUs
@larsoner - any opinion?

Is px the native value stored by Eyelink, not degrees (which could be converted to radians)?
I'm a bit torn because I agree it's not SI but if it's the native/standard eye-tracking unit then we should probably support it. It's a reasonable justification for deviating from the "everything in SI" standard we try to follow at least

Is px the native value stored by Eyelink, not degrees (which could be converted to radians)?

I'm not sure about "stored" as in stored in the EDF file. But the default unit for the (x,y) eyepos coords that is output to the ASCII files is pixels, usually with a 0.1 pixel resolution. Something like:
time	  xpos	 ypos
.001	  988.3	  532.6
.002	  987.5	  532.0

As a side note, eyelink can output visual degrees to the ASCII file instead, if the user specifies to. I think this is what the previously mentioned set_channel_type_eyetrack would try to address.

I noticed that BIDS already has a keyword specification for setting Eyetracking data type:
position data: EYEGAZE
pupil data: PUPIL
https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/03-electroencephalography.html
I think we should probably use these instead of eyetrack_pos and eyetrack_pupil to keep MNE consistent with the BIDS standard.

I think we should probably use these instead of eyetrack_pos and eyetrack_pupil to keep MNE consistent with the BIDS standard.

i dont think this is strictly necessary..  e.g. MNE also doesnt have different channel types for HEOG and VEOG, as in the BIDS standard.
setting the correct ch_type would happen under the hood in the BIDS IO (i.e. MNE-BIDS) anyway.
this being said, i am not against changing the ch_types to eyegaze and pupil if the majority thinks this is clearer. @larsoner what do you think?

it's not SI but if it's the native/standard eye-tracking unit then we should probably support it

well, as @scott-huberty said native is not so clear - depends on the eyetracker system and the user's  setup.
this has to look beyond this current PR as ppl might load data from different systems / convert stuff down the line.
yet, i think as of now it doesnt make sense to bother - if anytime later the plotted unit should be taken from info we can revisit the dafaults, but for now it might even make things more confusing if a user reads in data in deg but sees them plotted as px.. AUs are at least obviously defaults :)
i'll change it back

i dont think this is strictly necessary.

For sure, just think it could save MNE-bids (or someone in the future) the need to create a mapping between BIDS and MNE for this. But just a thought, happy to hear what others think.

it's not SI but if it's the native/standard eye-tracking unit then we should probably support it

well, as @scott-huberty said native is not so clear - depends on the eyetracker system and the user's setup.
this has to look beyond this current PR as ppl might load data from different systems / convert stuff down the line. yet, i think as of now it doesnt make sense to bother - if anytime later the plotted unit should be taken from info we can revisit the dafaults, but for now it might even make things more confusing if a user reads in data in deg but sees them plotted as px.. AUs are at least obviously defaults :)
i'll change it back EDIT: ah, you did it already :)

Yeah.. I think it was the last pain point in a couple of the CI's and I wanted to see some green ; )
I'm also totally happy to bring this back down the line if it makes sense to do so!

@scott-huberty  any comment on the tutorial btw? did you see it rendered?

this has to look beyond this current PR as ppl might load data from different systems / convert stuff down the line.
yet, i think as of now it doesnt make sense to bother - if anytime later the plotted unit should be taken from info we can revisit the dafaults, but for now it might even make things more confusing if a user reads in data in deg but sees them plotted as px.. AUs are at least obviously defaults :)

The convention so far in MNE is to try to read data from all systems to some SI unit during read. Then our Raw/Epochs/Evoked data attributes are always SI. So thinking from this perspective, I'm tempted to make the only supported .data unit for position radians and for pupil size meters. Would that work for Eyelink? If so, then let's start there and see if it will work for other systems.

edit: ah ok, CIs cant download the data apparently..

This SphinxWindows doc run doesn't use any data and shows 8 warnings (which we treat as errors)
https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=23588&view=logs&j=c04f1cb7-a28e-5e8a-4074-704b3a6c72b1&t=d58041c7-4107-5315-272d-0a36a2cb3945&l=2034
CircleCI on the other hand might be failing to protect against unauthorized users. But it says something different from when that's usually the case -- something about OAuth and logging out and back in. So you could try doing that on the CircleCI interface @scott-huberty . In the meantime I'll push an empty commit so that CircleCI hopefully uses my authorization level or whatever

/bin/bash: line 1: ./tools/circleci_download.sh: Permission denied

I think you somehow pushed a permissions change on the file. I'll fix it

Tutorial so far
https://output.circle-artifacts.com/output/job/d2d12f64-7538-4e52-b201-1ec3085529ee/artifacts/0/dev/auto_tutorials/preprocessing/90_eyetracking_data.html#sphx-glr-auto-tutorials-preprocessing-90-eyetracking-data-py

what do you think, is it good like this or should i rather squeeze it into set_channel_types ?

I did not look at the current implementation, but I would expect a dedicated eyetracking function for this. There isn't really a use case for changing channel types for other data -- unless you count rare stuff like remapping MEG channels from one system or MEG channel type to another, and we would definitely not do this with set_channel_types.
For fNIRS there are similar transitions, and these live in mne.preprocessing.* and mne.preprocessing.fnirs.*. I think it would make sense to add such eyetracking-specific stuff to a new mne.preprocessing.eyetracking.* namespace. For example, for unit conversions like pixels to degrees, something like mne.preprocessing.eyetracking.convert_eye_units(to, screen_size) or something would make sense.

We had a chat with @scott-huberty today on this issue. Maybe I can chime in. It got a bit complicated and I think to simplify the matter it is easier to split the problem in 3:

What would be the ideal case:
I think that there are three alternative outputs that may be important, depending on the application.
1.1 - Angle of the gaze with respect to a head reference (e.g., this would be what you'd like for EOG processing as it gives the measure of how much the eye moved)
1.2 - Angle of the gaze with respect to a screen reference (e.g., that is what would be needed for most psychometric study where "gaze movements" would be measured in degree of vision. Note that you can have no gaze movement while you have an eye movement, if you fix a point while moving the head)
1.3 - Pixels with respect to a screen reference (e.g., that is what you would want for an ET application where you are interested at what the participants is looking at on the screen)

What can be done given the constraints of EyeLink output:
For 1.1 :
1.1.1 If ASC generated using a head reference: Looks easy
1.1.2 If ASC generated using a screen reference: Looks complicated, but doable if (and only if) the head position was recorded (using the marker provided for that purpose with the EyeLink system)
For 1.2.:
1.2.1 If ASC generated using a screen reference: Looks complicated, but doable if (and only if) the head position was recorded
1.2.2 If ASC generated using a head reference: Looks complicated, but doable if (and only if) the head position was recorded
For 1.3:
1.3.1 If ASC generated using a screen reference: Direct
1.3.2 If ASC generated using a head reference: Looks complicated, but doable if (and only if) the head position and the screen resolution and position was recorded

What will be implemented:

That is for @scott-huberty to decide, but I'd suggest implementing 1.1.1 and 1.3.1 for that PR, and check the different conditions and raise NotImplementedErorr with informative messages for the other cases. That would ensure that the design can support the potentially useful use-cases and avoid spending much time coding "nice-to-have" features that nobody are using yet.
I do understand, however, that it does not solve your issue with using a non-SI unit @larsoner, but I think there is a genuine use-case for it because the conversion from degree (radian) of vision to pixel is non-trivial and not possible without additional info about the screen resolution and the position of the head and screen.

I think to simplify the matter it is easier to split the problem in 3:

To add context to “head reference” and “screen reference” @christian-oreilly  used to describe the 3 ideal scenarios, and respond to his suggestions:
These are the 3 possible data types for eye position that the Eyelink 1000 can produce (only one type will occur in any given ASCII file):

Pixels (default)
Head referenced Eye angle (HREF)
Raw pupil position (in the file because no calibration was done or by request)

“Screen reference” are data types 1 and 3
“head reference” is datatype 2.
if the data type is HREF, then converting this to radians and using a radian SI unit is fairly straightforward, as @christian-oreilly said in 1.1.1.  This is doable in this PR.
If the datatype is pixels or raw pupil positions (1.1.2 ), converting these to visual degrees or radians is nontrivial, and will be impossible for some files (without head position data). I think this is beyond the scope of this PR.
With regards to this and ideal case 1.3 (reading in pixel data)
This PR already reads eye position data that was measured in pixels. The MNE SI unit is AU. I think what is currenlty undecided is if it gets a special case pixel unit, an AU unit, or if MNE explicitly prevents reading in Pixel coordinate data.
FWIW Pixels are a common gaze coordinate system for trackers. I expect we will be seeing more of it if there are I/Os for other devices in the future..
As for pupil size "channels"…
Eyelinks metric of Pupil size is an arbitrary unit (it is NOT reported in mm).
It is possible to convert to mm, if and only if head distance data is in the file (and ideally, the head distance would be fixed during the recording).
So, converting this data to a Meter SI is not trivial, and will be impossible for some files.
My suggestion is to keep the pupil size SI unit as AU, since the data in the ASCII file is in AU (and this PR does not cover converting pupil data to mm).
To sum it all up..

For this PR, I can write code to convert the HREF datatype to radians (1.1.1). We can set its SI unit to radians.

My suggestion is to allow the reading of pixel datatype into MNE (1.3.1). Whether we create a special case unit for pixels, label it AU, or ban it altogether, is your call @larsoner

I suggest we keep the pupil size SI as AU.

check the different conditions and raise NotImplementedErorr with informative messages for the other cases

thought: We could create a convert_to_degrees and convert_to_mm flag in the read_raw_eyelink call, and raise NotImplemented or ValueError error if the conversion is not possible given the state of this PR (1.2.*).

For this PR, I can write code to convert the HREF datatype to radians (1.1.1). We can set its SI unit to radians.

Awesome!

My suggestion is to allow the reading of pixel datatype into MNE (1.3.1). Whether we create a special case unit for pixels, label it AU, or ban it altogether, is your call @larsoner

Let's just start with AU for now. We can add pixel later if we really want to do it.

I suggest we keep the pupil size SI as AU.

If we can convert to meters based on the file info we should. But if we can't, I agree it makes sense to store as AU.

We could create a convert_to_degrees and convert_to_mm flag in the read_raw_eyelink call, and raise NotImplemented or ValueError error if the conversion is not possible given the state of this PR (1.2.*).

Rather than make the read_raw_* more complicated, it might make more sense to add a mne.preprocessing.eyetracking.convert_pupil_size(...) that allows you to get from the AU/pixels originally in raw to meters for example. But I'd rather let this part be in separate PR driven by real use cases, as it's easier to think about and try out.

To add context to “head reference” and “screen reference” @christian-oreilly used to describe the 3 ideal scenarios, and respond to his suggestions:

I think some distillation/combination of this excellent summary and what @christian-oreilly wrote should land in tutorials/preprocessing/90_eyetracking_data.py. Bonus points if it's in this PR! But it can be a follow-up if necessary

If we can convert to meters based on the file info we should. But if we can't, I agree it makes sense to store as AU. [...]
it might make more sense to add a mne.preprocessing.eyetracking.convert_pupil_size(...)  in [a] separate PR.

Fine with me! : ) I'll leave the pupil SI unit as AU (since the data read from file are in AU), and we can address conversion to meters in a follow up PR.

I think some distillation/combination of this excellent summary and what @christian-oreilly wrote should land in tutorials/preprocessing/90_eyetracking_data.py.

Sure! Having this information documented is important. The information on pupil size units can be added to the preprocessing tutorial - and I was thinking of adding an IO tutorial in tutorials/io to further explain the Eyetrack channel types/units etc. Kind of Similar to the tutorials/io/30_reading_fnirs_data. Or do you think that is overkill? We can stick to the preprocessing tutorial if so.
Thanks, everyone.

I was thinking of adding an IO tutorial in tutorials/io to further explain the Eyetrack channel types/units etc. Kind of Similar to the tutorials/io/30_reading_fnirs_data. Or do you think that is overkill?

I also had this inclination when I wrote my comment. The io tutorial describing the data types and the preprocessing tutorial talking about how to get the types/form of data you want could make sense. Up to you!

was thinking of adding an IO tutorial in tutorials/io to further explain the Eyetrack channel types/units etc. Kind of Similar to the tutorials/io/30_reading_fnirs_data.

+1 for separate tutorials

most of what i would add has been said already.
as the others pointed out all unit conversions are not trivial and depend on many factors that only the user knows - for gaze this has been well pointed out, but also pupil size from AU to mm depends not merely on a conversion factor from area to diameter of a circle, but actually also on the gaze position (if ppl are not fixating in the center, it gets convoluted as the form of the recorded pupil changes from a rough circle to more elliptical) and unknown things like individual pupil shape, reflections etc.
area in au is probably the most acurate description, and diameter is always an approximation (still, some ppl / journals want it reported in diameter)
i'm just saying this, to point out that users will have to do their part - imo it is just important for the MNE implementation to support all/most of  the relevant units (and ideally offer flexible conversion) but whether things make sense simply has to be decided by the user.
coming back to gaze. sure, we could also dump the px and keep everything in either rad for angles or au for everything else (if this was the common notion right now? i lost track :) ) - from a user perspective i dont think it matters too much..  sure, at some point one wants to plot some heat maps etc, but whether MNE really needs to know if its px/pt/inches/whatever, i'm not sure (as ppl will have to supply additional information like the screen size for these plots anyway).
it might make sense though to think about possiblerequired settings like fixed head/tracker setup -> unit conversions and changes of reference (head/screen) is easily possible vs. other environment e.g. in mobile eye tracking, where conversions are not really possible and one has to stay in the measured unit
or at least a field that holds the information on  what the current reference is (head/screen)?
would be good to hear your thoughts on that @scott-huberty @christian-oreilly
from the previous discussions, i expected all preprocessing functions like unit conversions to happen in follow PRs anyway (and i think this makes sense).
so to sum up what's to be done in this PR before closing (minimum):

[ ] finalize decisions on which units are allowed and how they are stored in general
[ ] any other required general settings (e.g. reference type)?
[ ] the above, but specifically for the eyelink reader
[ ] point out current limitations etc in the tutorial (possibly in an additional io tutorial)

anything i missed?

anything i missed?

No I think that's a nice summary! So one by one:

finalize decisions on which units are allowed and how they are stored in general

I think for now just meters and rad if we have them, AU for everything else (pixels, etc.). We can add more later if needed, but hopefully we won't.

any other required general settings (e.g. reference type)?
the above, but specifically for the eyelink reader

Let's not for now. If 95%+ of setups use one or the other -- or if even @scott-huberty and @dominikwelke you both just use one of them! -- let's just work toward that use case. We can add refs later somehow driven by concrete use cases and applicability.

point out current limitations etc in the tutorial (possibly in an additional io tutorial)

Indeed I think this is mostly about adding some text about what is currently covered and what is not. Whether a list, table, or just sentences should be used to convey this information is up to whoever is going to write it. And keep in mind these are living documents so they don't have to be perfect in the first pass, just good enough to cover some sensible number of use cases and users...
Thanks for working through this everyone, I think the design is shaping up nicely!

Thanks for working through this everyone, I think the design is shaping up nicely!

Thanks everyone for the input.
@dominikwelke thanks for making that checklist, that will be a great tool to keep track of our TODOS.
Sorry I didn't respond about the tutorial earlier. It looks great - thanks for that!! While it's fresh on my mind after these discussions, I have been adding some info on the data types to a newio tutorial, and adding info on the pupil size in the preprocessing tutorial - after which I will ask for a review from you if you have time!

after which I will ask for a review from you if you have time!

sure, happy to have a look when its done!
i remembered two other minor things that i'll add to the checklist above:

populating info with stuff from the file header (meas_date, device_info, subject_info etc)
potentially renaming ch_types to eyegaze and pupil?

Good question
Meas_date

populating info with stuff from the file header (meas_date, device_info, subject_info etc)

@larsoner and @drammock , can you make a recommendation on how to set the meas_date in this situation?:
Eyelink ASCII files provide a timezone naive datetime of the recording. Out of these 3 options, which would you suggest?

Leave info['meas_date'] as None.
use the datetime for info['meas_date'] and force the timezone to be UTC, knowing that the meas_date will be technically incorrect.
allow the user to pass in a python ZoneInfo timezone, then set the meas_date in UTC using the user-passed timezone.

ch_types

potentially renaming ch_types to eyegaze and pupil?

@larsoner @drammock,  do you have a recommendation on whether we should adopt the BIDS terms for eyetracking channel types?

FYI We are using eyetrack_pos and eyetrack_pupil as eyetracking channel types, the BIDS community recommends eyegaze and pupil.

setting meta info

device_info, subject_info etc)

@larsoner @drammock, read_raw_eyelink will need to know what type of eye_pos data is in the file, so that it can convert to radians if possible. This could be useful for the users too.  The ASCII file declares the datatype, is there a field in the info structure that would be an appropriate place for this info? It will be a single string, one of 'Px', 'HREF', or 'raw pupil pos'

If not, i'll store it in an attribute pos_datatype.

@dominikwelke what other info did you have in mind? Do you think we should devote a follow up PR to this, rather than do it here?

meas_date ... use the datetime for info['meas_date'] and force the timezone to be UTC, knowing that the meas_date will be technically incorrect.

I think this is what we've done elsewhere. Just add to the Notes section that this is the case.

FYI We are using eyetrack_pos and eyetrack_pupil as eyetracking channel types, the BIDS community recommends eyegaze and pupil.

+1 for eyegaze and pupil. I think it's clear both relate to eye tracking so the eyetrack_ doesn't add much, and matching standards is always nice when it's easy enough to do

The ASCII file declares the datatype, is there a field in the info structure that would be an appropriate place for this info? It will be a single string, one of 'Px', 'HREF', or 'raw pupil pos'... If not, i'll store it in an attribute pos_datatype.

Anything we add needs to survive I/O round trip. The two places we can do this are in the data itself, or in known keys of raw.info, as you suggest -- so new attributes are kind of a no-go.
I'm thinking the best option would be info['chs'][ii]['coord_frame'], but it's not totally clear how the 'Px', 'HREF', 'raw pupil pos' would map into existing entries. We could add some new MNE_COORD_FRAME_* for it, though.

@dominikwelke what other info did you have in mind? Do you think we should devote a follow up PR to this, rather than do it here?

If at all possible, we should. It'll be easy enough for people to re-read and re-save with fixed/improved info I think, even if we take 6 months to a year to hammer out the correct values. Even the coord_frame stuff I suggest above could easily be in a follow-up PR!

hi @scott-huberty -

@dominikwelke what other info did you have in mind? Do you think we should devote a follow up PR to this, rather than do it here?

i would populate the info now, as its just the stuff from the first lines of the .asc file.
as written above i thought of meas_data (discussed above) and device_info (basically the model of the tracker and perhaps the recording software)
see doc:
device_info dict:

    type: str

        Device type.

    model: str

        Device model.

    serial: str

        Device serial.

    site: str

        Device site.

example header:

** CONVERTED FROM C:\Users\Display\Documents\ACAR_Q1K_protocol_2022_11_25_deploy\Q1K_ACAR_PLR_pfp_v1_deploy\results\scott\scott.edf using edfapi 3.1 Win32 Nov 21 2017 on Sat Nov 26 00:25:34 2022
** DATE: Fri Nov 25 17:10:47 2022
** TYPE: EDF_FILE BINARY EVENT SAMPLE TAGGED
** VERSION: EYELINK II 1
** SOURCE: EYELINK CL
** EYELINK II CL v5.15 Jan 24 2018
** CAMERA: Eyelink GL Version 1.2 Sensor=AG7
** SERIAL NUMBER: CLG-BFE01
** CAMERA_CONFIG: BFE01200.SCD
** RECORDED BY Q1K_ACAR_PLR_pfp_v1
** SREB2.2.299 WIN32 LID:96F1E67 Mod:2022.11.25 21:59 EST
**

hmm, type and model are not entirely straight forward to me..

should type just say something like 'eyetracker' @larsoner or is there a defined set of options?
the sensor entry from ** CAMERA: field should become part of the model, but the actual eyetracker model (e.g. EyeLink 1000) is missing (or do you actually have an eyelink II system, @scott-huberty ?)
serial  would be  ** SERIAL NUMBER:
site is missing in the header, i'd leave it blank

additionally, the whole header could go to info['description']

I'm thinking the best option would be info['chs'][ii]['coord_frame'], but it's not totally clear how the 'Px', 'HREF', 'raw pupil pos' would map into existing entries. We could add some new MNE_COORD_FRAME_* for it, though.

yes, i also thought of coord_frame.
and as said before i'd rather opt for screen reference and head reference
as @scott-huberty described above:

These are the 3 possible data types for eye position that the Eyelink 1000 can produce (only one type will occur in any given ASCII file):
Pixels (default)
Head referenced Eye angle (HREF)
Raw pupil position (in the file because no calibration was done or by request)
“Screen reference” are data types 1 and 3
“head reference” is datatype 2.

i think the combination of this coord_frame and the unit holds all relevant info for further processing, and this is a more general representation.
(actually screen reference is only correct for desktop mount eyetrackers - worldor target reference would be clearer -  but i think that's irrelevant)

additionally, the whole header could go to info['description']

+1 for just dumping this in info['description']. I doubt people will need it or care much and if they do they can parse the string

I think this is what we've done elsewhere. Just add to the Notes section that this is the case.

Great, I think this is the way @dominikwelke originally had the meas_date set, I'll change it back!

+1 for just dumping this in info['description']. I doubt people will need it or care much and if they do they can parse the string

+1 for this - If we decide we actually want to parse the header down the road we can

(actually screen reference is only correct for desktop mount eyetrackers - world or target reference would be clearer - but I think that's irrelevant)

If I'm not mistaken I think @larsoner suggested we don't set reference type for now (see below)?  I just wanted to store the datatype as it is reported from eyelink (pixel head referenced eye angle, raw pupil data), as this will be sufficient for what this I/O will need (if HREF, convert to radian), and for the user to double-check the datatype if they need to:

3. any other required general settings (e.g. reference type)?
4. the above, but specifically for the eyelink reader

Let's not for now. If 95%+ of setups use one or the other -- or if even @scott-huberty and @dominikwelke you both just use one of them! -- let's just work toward that use case. We can add refs later somehow driven by concrete use cases and applicability.

I think this is what we've done elsewhere. Just add to the Notes section that this is the case.

Great, I think this is the way @dominikwelke originally had the meas_date set, I'll change it back!

To get into bonus territory you could add a tzinfo='utc' arg to the reader that the user could set to tzinfo='US/Eastern' for example or whatever that would un-naive the given time then convert it to UTC correctly using an optional dependency on pytz. I think it would only be a few lines and it would get us correct times. But again this could be a follow-up since the default 'utc' will be a no-op / maintain current behavior.

I just wanted to store the datatype as it is reported from eyelink (pixel head referenced eye angle, raw pupil data), as this will be sufficient for what this I/O will need (if HREF, convert to radian), and for the user to double-check the datatype if they need to

I am not sure I follow the details, but I think as long as we extract as much relevant info as possible to relevant places we're good, and critically it doesn't have to all be completed in this PR / all at once... so I'm happy to omit this part entirely for now if you want.

I think this is what we've done elsewhere. Just add to the Notes section that this is the case.

Great, I think this is the way @dominikwelke originally had the meas_date set, I'll change it back!

To get into bonus territory you could add a tzinfo='utc' arg to the reader that the user could set

Good idea!

I am not sure I follow the details, but I think as long as we extract as much relevant info as possible to relevant places we're good, and critically it doesn't have to all be completed in this PR / all at once... so I'm happy to omit this part entirely for now if you want.

Yeah, I'll just omit it for now, that way I don't take up the coord_frame key and we can have a dedicated PR to hash out the details regarding the setting meta info for eye tracker data 🙂

Sorry for the  delay! Had to address some paper revisions last week.
For Eyelink files with  'HREF' (head referenced) position data,  read_raw_eyelink will automatically convert the values to radians and set the FIFF unit to radian.
Here is what an example file looks like, it's a text reading task.

Compare that, to the same exact file with the position data in pixels instead:

I think scalings_plot_raw in mne.defaults is calibrated for pixel data. If we were to change this to a more appropriate value for radians, then the files with HREF (i.e. units in radians) position data will look nice on plots and files with pixel position data would be blown out. Not sure what to do about that.. just FYI!

If we were to change this to a more appropriate value for radians, then the files with HREF (i.e. units in radians) position data will look nice on plots and files with pixel position-data will look blown out when plotting. Not sure what to do about that.. just FYI!

Hmm... so far in MNE we've never had a channel / "coil" type with more than one possible unit. But maybe it's time to deal with that case. It'll take more (a lot of?) work under the hood, though, so we should just go with the scaling people are most likely to use for now (radians hopefully) and deal with the other later.

If we were to change this to a more appropriate value for radians, then the files with HREF (i.e. units in radians) position data will look nice on plots and files with pixel position-data will look blown out when plotting. Not sure what to do about that.. just FYI!

Hmm... so far in MNE we've never had a channel / "coil" type with more than one possible unit. But maybe it's time to deal with that case. It'll take more (a lot of?) work under the hood, though, so we should just go with the scaling people are most likely to use for now (radians hopefully) and deal with the other later.

Yeah.. it's a weird situation for sure, since these aren't different channel types, but different derivatives of the same channel/coil.
I'll make the default scalings work with radians, and I'll include an example in the tutorials, that uses the scalings argument in raw.plot(), to get preferable scalings for pixel data.

@dominikwelke , we are looking green!  Do you want to review the built documentation when you have some time?

@dominikwelke if you don't have time I can take a look early next week. It would be good to get some variant of this in soon-ish so we have some time to use it a bit before release (which should be in about a month).

Hmm... so far in MNE we've never had a channel / "coil" type with more than one possible unit. But maybe it's time to deal with that case. It'll take more (a lot of?) work under the hood, though, so we should just go with the scaling people are most likely to use for now (radians hopefully) and deal with the other later.

not saying we should do this now, but just as a comment @larsoner @scott-huberty :
one obvious workaround to avoid headaches would be to actually have different coils under the hood; e.g. FIFF.FIFFV_COIL_EYETRACK_POS_PX/ FIFF.FIFFV_COIL_EYETRACK_POS_DEG with each having just one fitting unit.
(or FIFF.FIFFV_COIL_EYETRACK_POS_HREF with deg/ FIFF.FIFFV_COIL_EYETRACK_POS_EXTREF with AU or some other meaningful solution)
in this scenario there would be some juggling to match MNE ch_type and FIFF coil_type but this could be done in the reader function and deeper changes should be avoided.

one obvious workaround to avoid headaches would be to actually have different coils under the hood; e.g. FIFF.FIFFV_COIL_EYETRACK_POS_PX/ FIFF.FIFFV_COIL_EYETRACK_POS_DEG with each having just one fitting unit.

I think this is my preferred solution. Then we stay with a unit-to-coil/channel type mapping, which will make life easier for us.

Agreed if we can do it in the next week or two that's fine!

No objections from my side. I could change the fiff pr this weekend

That would be super helpful since I don't have as much experience creating the new channels/coils, thanks @dominikwelke !

i created unique coils for all data modality/unit pairs we have so far
(also left space for potential future changes, eg gaze position in metric units, who knows)
    eyetrack_pos_deg    		400  "Eye-tracking gaze position (in DEG vis angle)"
    eyetrack_pos_px    		401  "Eye-tracking gaze position (in PX on screen)"
    eyetrack_pupil_area_au  	410  "Eye-tracking pupil size (area, in AU)"
    eyetrack_pupil_diameter_au  	420  "Eye-tracking pupil size (diameter, in AU)"
    eyetrack_pupil_diameter_mm  	421  "Eye-tracking pupil size (diameter, in MM)"

how should we move on with the MNE ch_types now?

option 1 - separate ch_types for all coils:
(eyetrack_pos_deg, eyetrack_pos_px, eyetrack_pupil_area_au,eyetrack_pupil_diameter_au, eyetrack_pupil_diameter_mm)

option 2 - ch_types for all data modalities (one level higher):
eyetrack_pos,eyetrack_pupil_area,eyetrack_pupil_diameter

the 1st would be clearest, and starigth forward to implement.
the 2nd would probably be nicer from the API side, but requires a bit more code to sort things out
defaulting to pos data in PX, and pupil data in AU might make sense in channels.py, while read_raw_eyelink and set_channel_type_eyetrack could deal with accurate setting

I think in fNIRS data you can convert between raw measurements (fnirs_od) to oxyhemoglobin (hbo) and deoxyhemoglobin (hbr). Are these different channel_types, derived from the same "coil" (optode/channel/source?) during online acquisition?

They end up as different coil types
https://github.com/mne-tools/mne-python/blob/bf2502166eb15626c1205accc2d2d467535b8d93/mne/io/constants.py#L912-L917

Unused in this PR (we are not converting pupil size to MM. EDIT: Also, if we do, we would convert it to Meters, because that is the SI unit. If possible/feasible this will be done in a separate PR):

Indeed we should not have mm but rather m because we want SI when we can

I did not look at the entire PR yet and only read the couple of last messages, but channels in MNE do have a unit variable that can be retrieved in .info["chs"]:
[{'scanno': 376,
  'logno': 61,
  'kind': 102 (FIFFV_BIO_CH),
  'range': 0.00030517578125,
  'cal': 0.002142000012099743,
  'coil_type': 0 (FIFFV_COIL_NONE),
  'loc': array([0., 0., 0., 1., 0., 0., 0., 1., 0., 0., 0., 1.]),
  'unit': 107 (FIFF_UNIT_V),
  'unit_mul': 0 (FIFF_UNITM_NONE),
  'ch_name': 'fakeAcce',
  'coord_frame': 0 (FIFFV_COORD_UNKNOWN)}]

Maybe that would be a good placeholder for pixels vs meters?
Note also the unit multiplication factor, but to my knowledge it isn't used in the MNE code base . It's always set to 0 and MNE assumes the units to be SI. Thus mm should probably be meters.
P.S: Big thanks for adding support for eyetracking data, looking forward to using this PR for our EyeLink systems!

@mscheltienne yes in theory I think the most correct thing to do would be to have a single channel type/coil and just change the ch['unit'] when converting...
I think I've flip-flopped a bit on this. But this is maybe the best long-term solution...
So maybe what we do is:

Have a single coil type, but change ch['unit']
Fix plotting functions gradually to actually take the ch['unit'] into account

This PR could just have (1)... and for now it will have to do some scalings=... in raw.plot for the AU / non-meter unit, maybe also for pixels. Then a follow-up PR can improve raw.plot and evoked.plot etc. to account for the ch['unit'] properly.

Indeed we should not have mm but rather m because we want SI when we can

I created coils for the units researchers use, not SI.. otherwise eyetrack_pos_deg would be radians as well.
but if we roll back this doesnt matter anyway :)

channels in MNE do have a unit variable that can be retrieved in .info["chs"]

@mscheltienne  quick explanation - we used the ch['unit'] field throughout the whole PR journey. our reason to consider other options now is, that the current MNE implementation cannot/does not use this entry at all (as so far, there are only channel types with a single unit option.)
plots etc are hard coded to retrieve this one possible unit and we would need to touch all kinds of functions and probably deeper implementations to be able to handle different units for a single ch_type.
i agree that it would be nicer to proceed with the way we started and have flexible units for a channel type, but i cannot really judge how much work this would mean.. you should decide ;)

I created coils for the units researchers use, not SI.. otherwise eyetrack_pos_deg would be radians as well.
but if we roll back this doesnt matter anyway :)

Please do not do this -- the constants have to do with how data are stored and accessed on disk. Even MEG data gets stored in Tesla (T) even though they are always displayed in units of fT or nT. Let's keep the constants for data as SI as possible, and make the plot functions plot with scaling people like to see

i agree that it would be nicer to proceed with the way we started and have flexible units for a channel type, but i cannot really judge how much work this would mean.. the core devs should decide ;)

I think I'm back to being for what we had before -- a single coil_type and we'll work out properly supporting ch['unit'] gradually. Thinking about the plotting functions I think we can fix them one by one in individual PRs. This first PR can just have a workaround with scalings=... for the one that is not SI (AU or pix or something?) and hopefully not need it for the one that is in m on disk (and displayed as mm in the plots -- this is doable given the constants we can set here).
@dominikwelke if this is not clear if you let me know the last commit we had it done this way then I can revert some stuff and try to make a version version work from there (but maybe you had this already with a scalings hack). Hopefully it's not too bad!

i didnt change anything in this PR as i first wanted to hear WHAT to change ;)
i will simply roll back the commit in the FIFF PR

i will simply roll back the commit in the FIFF PR

Okay so in that case, is this PR ready to go from your end @dominikwelke ? So we could:

Review/merge fif-constants PR
Update this PR to use official fif-constants commit hash in PR
Review code
Merge
In follow-up PRs, fix scalings in plot functions by actually accessing ch['unit'] to check if it's radian or AU (pix)

Does that sound right?

@larsoner should i also remove pxor leave it in?

I think we probably do want that as a ch['unit'] but not as a separate coil type

ok then, this is basically the current status.
in fiff there is

1 channel type (eyetrack)
2 coil types (eyetrack_pos or eyetrack_pupil)
4 relevant units (au, rad, px, m)

in MNE we have 2 ch_types though (eyetrack_pos or eyetrack_pupil)
MNE's ch['unit'] field can be set to all entries specified in the fiff constants. this is px or rad for gaze position (eyetrack_pos) and au or m for pupil area/diameter (eyetrack_pupil; transcription to metric values not implemented yet)

Yes that sounds right!

in MNE we have

I would just add to this:

pick functions should take a single eyetrack=... kwarg just like we have fnirs=.... The user can do for example pick_types(eyetrack='pupil') or eyetrack='pos' for example to get just one type, True to get both types, etc. just like for fNIRS.

I assume this is how it's implemented already but just wanted to add it for completeness.

i could do that tomorrow if you dont want to @scott-huberty

It would be helpful if you have the time! thanks @dominikwelke

This PR could just have (1)... and for now it will have to do some scalings=... in raw.plot for the AU / non-meter unit, maybe also for pixels.

Don't know if I'm totally following But sounds fine to me - I didn't touch inst.plot() so if the user reads in data that isn't in radians, they will need to pass custom scalings to inst.plot().
FWIW I included a message in the logger of read_raw_eyelink, if the datatype detected is in pixels, the logger tells the user to pass scalings=dict(eyegaze=1e3)  when using raw.plot(). It's an Imperfect solution but as far as I can tell, it's the best we can do until we refactor the code in inst.plot() (I assume this is best left for a follow up PR).

pick functions should take a single eyetrack=... kwarg just like we have fnirs=.... The user can do for example pick_types(eyetrack='pupil') or eyetrack='pos' for example to get just one type, True to get both types, etc. just like for fNIRS.

I assume this is how it's implemented already but just wanted to add it for completeness.

I think so, but I'll double check.
IMO this PR is ready for review.
Thanks everyone!

as i now also removed the non-SI degand mm units from the FIFF PR, they need to leave constants.py, channels.py, and preprocessing/../eyetracking.py too - i could do that tomorrow if you dont want to @scott-huberty

It would be helpful if you have the time! thanks @dominikwelke ... IMO this PR is ready for review.

@dominikwelke let me know when you push the suggested fif-constant change, and I'll review and merge that PR over in fif-constants and review this one as well!

The fiff pr is as described above, you can review it @larsoner
Am 13. März 2023, 19:50, um 19:50, Eric Larson @.***> schrieb:

as i now also removed the non-SI degand mm units from the FIFF PR,
they need to leave constants.py, channels.py, and
preprocessing/../eyetracking.py too - i could do that tomorrow if you
dont want to @scott-huberty

It would be helpful if you have the time! thanks @dominikwelke ...
IMO this PR is ready for review.

@dominikwelke let me know when you push the suggested fif-constant
change, and I'll review and merge that PR over in fif-constants and
review this one as well!
--
Reply to this email directly or view it on GitHub:
https://github.com/mne-tools/mne-python/pull/11152#issuecomment-1466748102
You are receiving this because you were mentioned.
Message ID: @.***>

@larsoner @scott-huberty
my last merge seems to have messed with circleci oauth..
i think we had that before - how can i fix it?

I just pushed an empty commit to work around whatever Circle's problem is

https://output.circle-artifacts.com/output/job/bfe655d8-d69a-459c-a330-fbce879dd9c5/artifacts/0/dev/auto_tutorials/io/70_reading_eyetracking_data.html#sphx-glr-auto-tutorials-io-70-reading-eyetracking-data-py
https://output.circle-artifacts.com/output/job/bfe655d8-d69a-459c-a330-fbce879dd9c5/artifacts/0/dev/auto_tutorials/preprocessing/90_eyetracking_data.html#sphx-glr-auto-tutorials-preprocessing-90-eyetracking-data-py

@scott-huberty @dominikwelke let me know if you're both happy with these and I'll do a full review. I think we can assume the fiff-constants PR will be merged, we just need to wait for the other maintainers to find a minute to look and comment, which make take a couple of days. But maybe we can merge by Friday if all goes well!

thanks @dominikwelke for making those changes!

let me know if you're both happy

LGTM. @larsoner no pressure but just FYI - I will be out of office next week but I do have time this week to address any questions/revision requests for this PR.
One thing - It might be good to test our method the HREF to radian conversion, but I don't have a file in datasets.testing to do this. We do have one in datasets.misc, though when I tried to call the misc datapath from the testing module, MNE complained.

We do have one in datasets.misc, though when I tried to call the misc datapath from the testing module, MNE complained.

Yeah don't use misc the CIs won't use it, so it's not all that useful. Can you do a 1 sec recording and update mne-testing-data with it instead?

Yeah don't use misc the CIs won't use it, so it's not all that useful. Can you do a 1 sec recording and update mne-testing-data with it instead?

The file I have is only like 5mb as is.. Can I just add it to mne-testing-data? Unless you want it to be 1mb ; )

Yes 1mb is better than 5mb. I know it seems trivial but we test a lot of stuff and it adds up...

Yes 1mb is better than 5mb.

or you could just remove the last ~80% of lines from the asc file you have @scott-huberty ;)

I'm not sure why - the minimal 3.8 CI is returning a module import error for: nibabel on many of the beamformer tests.

this doesnt seem like our fault, but test_radianseems to have failed too?

Argh I introduced those regressions and will fix them in a separate PR. Don't know why they're only being caught now, though!

this doesnt seem like our fault; all the other CIs are fine
but test_radianseems to have failed too: no pandas?! seems more like the testing environment is outdated or broken

ahh ya thanks for pointing that out -- I'll add the requires_pandas decorator and push.

🎉🎉🎉  100 commits  🎉🎉🎉

https://github.com/mne-tools/fiff-constants/pull/39 is in, feel free to update this PR and ping me when it's green -- I think we can merge it then!

it's green and i dont have further comments right now

argh.. getting another nibabel import error in mne/simulation/tests/test_raw.py::test_simulate_raw_bem[testing_data]

Pushed a commit for the importorskip and marked for merge when green. Thanks in advance @dominikwelke @scott-huberty , this will be very useful!

Pushed a commit for the importorskip and marked for merge when green. Thanks in advance @dominikwelke @scott-huberty , this will be very useful!

Thank you!!!
Big thanks to everyone for all the work on this!!!


[AutoRelease] t2-durabletask-2024-11-19-44084(can only be merged by SDK owner)

https://github.com/Azure/sdk-release-request/issues/5725
Recording test success

issue link:https://github.com/Azure/sdk-release-request/issues/5725

/azp run prepare-pipelines


Slow as a snail

Updated to new version. When I click ankimote, it takes 10+ seconds for QR code to come up. When in tap mode, taps take 10+ seconds to register as well. This is on latest release I downloaded from github 8/17,  5e41f2a.

Reverting to f72ccf9 fixed the issue.

That's hard to interpret since the none of the code for starting the server and having the QR show up has been changed from the 2020 release that works for you. But glad you've found a fix!


publish<EMAIL_ADDRESS>

Requested by: @buenaflor
Merge target: (default)
Quick links:

View changes
View check runs

Assign the accepted label to this issue to approve the release.
Leave a comment containing #retract under this issue to retract the release (original issuer only).
Targets

[ ] npm
[ ] gcs
[ ] registry
[ ] docker[release]
[ ] docker[latest]
[ ] github
[ ] gh-pages

Targets marked with a checkbox have already been executed.  Administrators can manually tick a checkbox to force craft to skip it.

@kahest @buenaflor -- this should have been a minor release -- please try to adhere to semver in the future thanks!

you're right, will do!

ah, I was following what the previous flutter version upgrade was released as
https://github.com/getsentry/craft/blob/master/CHANGELOG.md#122

yep I also complained about that one incorrectly doing semver as well: https://github.com/getsentry/publish/issues/2250


Working with React 16.13.0

I ran to the error about Invalid hook call when created the project with React 16.13.0
Error
Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:
1. You might have mismatching versions of React and the renderer (such as React DOM)
2. You might be breaking the Rules of Hooks
3. You might have more than one copy of React in the same app
See https://reactjs.org/link/invalid-hook-call for tips about how to debug and fix this problem.

How to solve this issue? or we just need to use React 17.0.0 or the newest?

Please upgrade to 17 and the error should go away. let me know how it pans out.

Yeah, there is no error when I've upgraded to 17.0.0 but kinda curious is any possible to use 16.x.x version?

sure will look into what is causing this.

@xeusteerapat just published the new version 1.2.1. should work for 16 and 17 now.

Hi, there is no error like the previous one after upgraded to the latest version.


Moving quant primitives from ao_benchmark to protoquant

Stack from ghstack (oldest at bottom):

#14
#13
#12
-> #11

This commit begins work on gpu int8 quant primitives that that are
usable by researchers/developers. existing primitives either do not
share a mental model with existing pytorch ao quantization framework
(protoquant) or are specifically tailored to a single use case
(ao_benchmark). This commit's code is copied from pytorch/ao_benchmark,
credit goes to @vkuzo

should the tests be moved in this PR as well?


AttributeError: 'Categorical' object has no attribute 'sort'

It seems like that the codebook is not able to handle Categorial data.
https://github.com/mhowison/codebooks/blob/50b6799c4ca46dea6f086722422e5e62401e0539/codebooks/variable.py#L18
The full error is:
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\IUK\Python\lib\site-packages\codebooks\__init__.py", line 20, in htmlbook
    var = Variable(df[varname], desc.get(varname, ""))
  File "C:\IUK\Python\lib\site-packages\codebooks\variable.py", line 18, in __init__
    self.values.sort()
AttributeError: 'Categorical' object has no attribute 'sort'

I would suggest to check if the column is a ordered categorial - then you can sort(). If not, do not sort.

https://github.com/mhowison/codebooks/issues/2

Removed the sort in v0.0.6


The channel selection of auxiliary advertisments in extended advertisments

Hi, is there any planned support for some heuristic functionality that would dynamically select channels for auxiliary advertisements? We found out that channel 0 is selected as the default for any extended advertising payloads which might be quite unreliable if there is a lot of inference on this particular channel.
Thanks,
Lukas

Advertising extensions is work in progress, currently no one is working  is being done on the auxiliary advertising channels. Contributions to add it is welcome.


Warns 'Hydrogen is outdated' while it's up-to-date

For some reason it warns 'Hydrogen is outdated!
Get the latest version by going to Plugins > Manage Plugins > Click "Update" button under "Hydrogen"
OR
Get the latest release from the github page (https://github.com/Synthetic-Dev/Hydrogen/releases)!' while the plugin is up-to-date.

I've tried relaunching studio but it always warns that.

Apologies! I never got round to publishing the latest version of the plugin after Roblox came back online from the outage! Will publish the update now.


probe-rs-debugger Does not halt on breakpoints when using 'launch' and not flashing.

Describe the bug
probe-rs-debugger Does not halt on breakpoints when using 'launch' and not flashing.
To Reproduce
Steps to reproduce the behavior:

Using VSCode, add a launch.json that does uses:  "flashingConfig": {"flashingEnabled": false}
Set a breakpoint in main(), before the loop'ing part of the function.
Build, and manually flash the binary to the target.
Start a debug session with F5.

Expected behavior
I would expect the target to launch the binary app on the target, and then to halt at the breakpoint.
Additional context
This used to work, so I can only conclude that one of the recent updates broke this functionality. To be investigated.

@noppej Can you see if you're getting warnings in the Debug Console as outlined in #1532 ? I saw this symptom too.

@noppej Can you see if you're getting warnings in the Debug Console as outlined in #1532 ? I saw this symptom too.

@mubes, as per my comment in #1532, I think the two issues are unrelated.  Feel free to DM if you want to discuss collaborating on a fix for that one.


Discord invite invalide.

Can you update invite link?

Thanks! It looks like the old invite links are disabled. I've already updated the link in the next release, which will be published soon.
But here is the new invite link until then: https://discord.gg/xhBCC7eGKw


Update Cost Function

The costs for Routes and Vehicles are not in sync. Use it in update cost function for faster results.

Cost sync done. Demands problem raised in #2


Slider attribute is adding filter value twice to url

We have an attribute set to be displayed as a slider which continuously adds the value you select to the url (even though there is already a value present);
Example
https://some-url/products?slider=1-2
If you would then select a new value for the "slider" attribute you would get
https://some-url/products?slider=1-2&slider=1-3
This keeps repeating everytime you select a value.
I traced the code back to /view/frontend/templates/catalog/layer/filter/js/slider-filter-js.phtml in this section
_applyRange: function () {
    var range = {
        from: this._getOriginalValue(this.minprice) * (1 / this.rate),
        to: this._getOriginalValue(this.maxprice) * (1 / this.rate),
    };
    const searchParams = () => {
        let searchParams = new URL(window.location).searchParams
        searchParams.delete('price')
        searchParams.delete('q')

        return searchParams.size !== 0 ? '&' + searchParams.toString() : ''
    }
    this.url = this._unescape(this.options.urlTemplate.replace('<%- from %>', range.from).replace('<%- to %>', range.to) + searchParams());
},

Also unsure why the "price" and "q" are already being removed;
It could seem logical for price because this bug would also apply for that attribute if it's displayed as a slider, but "q" I have no idea.
Using
smile/elasticsuite: <IP_ADDRESS>
hyva-themes/magento2-smile-elasticsuite: 1.2.3
shellpea/magento-advanced-elasticsuite-catalog: v1.2.6

@koenner01 Hi, thank you for your feedback, indeed, the word “price” is incorrect to use. For a quick fix you can override the specified file and replace the price with this code
searchParams.delete(this.options.requestVar)

and it will work for every type of slider you use 🙏
p.s. I can’t remember why I need to remove 'q', but there was some case in which it caused an error, if this causes unexpected behavior, you can remove this line, I will try to check if this causes an error in the default hyva theme and the next release is 1.2.7 will be released with these changes