print won't print without an argument

print()  # prints newline in python, nothing in kuroko
print("")  # prints in both

# also...
print("hello", end=" ")
print(end="world")  # this only works in python
print()
print("hello", end=" ")
print("", end="world")  # this works in both

the case with print(end="blah") not printing anything had me especially confused when I was porting a script, as I have a (bad?) habit of using that form.
this is with Kuroko 1.2.3 (commit 650f324) and a tiny patch to build with MSYS2 (MSYSTEM=MSYS).

Ah, looks like a pretty straightforward case of just completely neglecting argc == 0... I could have sworn this was being handled at some point, but likely before print become a normal function.
diff --git a/src/builtins.c b/src/builtins.c
index ba8a711..ef0f46e 100644
--- a/src/builtins.c
+++ b/src/builtins.c
@@ -512,6 +512,11 @@ KRK_FUNC(print,{
 			endLen = AS_STRING(endVal)->length;
 		}
 	}
+	if (!argc) {
+		for (size_t j = 0; j < endLen; ++j) {
+			fputc(end[j], stdout);
+		}
+	}
 	for (int i = 0; i < argc; ++i) {
 		KrkValue printable = argv[i];
 		if (IS_STRING(printable)) { /* krk_printValue runs repr */

As for the MSYS patch, I will take a look, but I suspect a more direct fix would be to change the bit in the Makefile that's checking for mingw so that MSYS takes the same path. Native Windows builds without cygwin or the like should work well.

ah, I wasn't suggesting the patch so much as I was including it for reproducibility.
taking the other branch in the Makefile almost works; the pthread library is named differently here:
-WINLIBS= -l:libwinpthread.a
+WINLIBS= -l:libpthread.a

and gcc still warns about ignoring the "hidden" visibility attribute.
also, I have a few other small issues to report relating to built-ins. would you prefer if I made individual github issues like this, or something else?

is this supposed to be ifndef _WIN32?

No, that's for Sortix, which explicitly does not have putenv.

also, I have a few other small issues to report relating to built-ins. would you prefer if I made individual github issues like this, or something else?

Let's do a separate issue for each function.

Closing as fixed in 21b459919b98c084a9ab44da4ebc6ad99d6e09dd


Add reservations counter

The following changes have been made to this PR:

A specific function is created to calculate reservations
Reservations pop-up displayed the correct number.
Counter is implemented as a separate module.
A counter function looked for specific DOM elements
A counter function covered all the edge cases

Hi @Metaverse-Nabeel   👋,
Your project is complete!  There is nothing else to say other than... it's time to merge it :shipit:
Congratulations! 🎉
Highlights

API works perfectly 👍
Asynchronous code used well 👍

Optional suggestions
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will improve your application.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.

As described in the Code reviews limits policy  you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.


Implement texture filtering (and honor PF_NoSmooth)

Quick attempt at adding texture filtering.  It's been a minute since I've done anything with D3D9, but I think this is all OK.  It does look quite a bit better.
Pulled the device caps in initialize to get MaxAnisotropy.  I figure anything that could run RTX Remix probably won't have any issues with anisotropic filtering.
#4

Perfect, you're just missing the restore of the render state in PopDeviceState(), but I'll add that one.
I'll also create a CONTRIBUTORS file, what name shall I put in there? RCGodward?

Released as per https://github.com/onnoj/DeusExEchelonRenderer/releases/tag/v0.1.18


connect to kong admin fail

error: KongProxyController request error undefined
error: Sending 500 ("Server Error") response:
{ error:
{ Error: connect ECONNREFUSED <IP_ADDRESS>:8001
at Object._errnoException (util.js:992:11)
at _exceptionWithHostPort (util.js:1014:20)
at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1186:14)
code: 'ECONNREFUSED',
errno: 'ECONNREFUSED',
syscall: 'connect',
address: '<IP_ADDRESS>',
port: 8001 } }

@kkxxr66573 , it seems that <IP_ADDRESS>:8001 is not accessible. This doesn't seem to be an issue with Konga.
Make sure that Kong's admin api is accessible from <IP_ADDRESS>:8001.
What is the connecton url you have setup in Konga?

Closing due to inactivity

same issue

If you are konga in docker on localhost and trying to access kong running on localhost, then don't use localhost. Instead, use host.docker.internal (works on mac, windows). for further info check


MoveIt setup assistant generates files which do not run on Windows without modification

<param if="$(arg load_robot_description)" name="$(arg robot_description)" command="xacro --inorder  '$(find arm_support)/urdf\workcell.xacro'"/>

Should remote the single quote on Windows

Hope this could be addressed by this: https://github.com/ros/ros_comm/pull/2051

Addressed up stream


slient overflow occurs in tf.range leading to incorrect result, here is a possible fix

Issue type
Bug
Have you reproduced the bug with TensorFlow Nightly?
Yes
Source
source
TensorFlow version
2.15.0
Custom code
Yes
OS platform and distribution
No response
Mobile device
No response
Python version
No response
Bazel version
No response
GCC/compiler version
No response
CUDA/cuDNN version
No response
GPU model and memory
No response
Current behavior?
Following the documentation https://www.tensorflow.org/api_docs/python/tf/range, tf.range should have consistent result with np.arange.
However, given the following inputs, tf.range outputs tf.Tensor([1136033460], shape=(1,), dtype=int32), which is incorrect
The correct result is np.arange's output [1136033460 -713794229].
I did some analysis and find the root cause lies in here:
https://github.com/tensorflow/tensorflow/blob/06e37b8d7f2ba0338efd0f588f068122414f0722/tensorflow/core/kernels/sequence_ops.cc#L100
When computing the limit - start, the actual value overflows.
To solve this problem, we can change the original code to a new one by applying the division first
origin:
Eigen::numext::ceil(Eigen::numext::abs((limit - start) / delta));

new:
Eigen::numext::ceil(Eigen::numext::abs(limit / delta - start / delta);

Standalone code to reproduce the issue
import numpy as np
import tensorflow as tf
start =<PHONE_NUMBER>
end = -2110457150
step = -1849827689
out = tf.range(start, end, step)
print(out)
out_np = np.arange(start, end, step)
print(out_np)

### Relevant log output

_No response_

Your analysis and solution seem valid. Open a PR if you'd like, otherwise I can handle it

I am afraid I don't have time in these few days, I would appreciate it if you can help me to open one.

Hi @QuantumCoder4 , @Aloqeely
I acknowledge the issue of overflow and attached gist for reference.The proposed fix might work it seems. Please feel free to raise a PR.

Hi @Aloqeely ,
I have a question. What happens if the value of delta =1 here keeping other two params same. It still might cause overflow ?

Line 83 in sequence_ops.cc:
if (delta > 0) {
      OP_REQUIRES(
          context, start <= limit,
          errors::InvalidArgument(
              "Requires start <= limit when delta > 0: ", start, "/", limit));
    }

In that case, this if statement would get triggered since delta = 1, and then start <= limit would get evaluated to false so the function will raise an error

The proposed fix above won't work - it leads to different behavior.  See the discussion in the (now closed) PR #64144.
Someone can pick that up if they like.

Is it ok if I take this over?

I believe I have a solution for this. Can open up a PR in bit if @brandong512 isn't still actively working on it

Reopening as the fix in #76252 is getting rolled back due to it introducing a signed integer overflow issue.

Reopening once more, see https://github.com/tensorflow/tensorflow/pull/77018#issuecomment-2412560383


Burger menu icon broken

It looks like the burger menu icon is broken, even on the demo site:
Firefox 37.0.1

Chrome 41.0.2272.118

I get this issue with all latest versions of Safari, Chrome and Firefox. It looks like it happens only when the Ghost site is behind a web proxy. Local setups, using <IP_ADDRESS> work fine for me. Could this be related to isse #186?

Not sure if it is really a problem with the font or a problem with caching. One of my test sites magically started to work. I will put some more time into it to further test and debug this issue.

Hi @jbfriedrich, it looks like it might be the same bug you linked to. Could you try out the master version of Casper (Download zip) and see if that solves it?

The master version of Casper works out of the box for me. But now the old version works for me as well. It looks like it was a very weird caching issue. Even though the web server was restarted several times and several web browsers were used (and cache was emptied several times). The site http://demo.ghost.io still has a broken menu burger icon for me though.


[ENHANCEMENT] Add dslx formatter to the extension

Could you consider adding the DSLX formatter to the extension, please? I think it would be helpful.
https://github.com/google/xls/blob/main/xls/dslx/dslx_fmt.cc

Formatter can be built and used similarly to the LSP
bazel build //xls/dslx:dslx_fmt

@mczyz-antmicro we can also plug the autofmt() API under the language server implementation since it's available as a library.

Even better!


Proper way to identify image format

What is the proper way to identify the image format (working across iOS and Android)?
Should it be inferred from response.uri like in here (the relevant snippet copied bellow)?
ImagePickerManager.showImagePicker(options, (response) => {
  ...
  const re = /(?:\.([^.]+))?$/,
        type = re.exec(response.uri)

  if (type[1] === 'jpg' || type[1] === 'jpeg' || type[1] === 'png' ){
    ...
})

If that code is working for you then yes I would say that is the proper way.

Well I haven't tested it thoroughly (only iOS simulator and iOS device so far)...

Can we confirm this is working, is it possible to add this in the documentation using base64 with a hardcoded mimetype can bring problems.


feat: finish command handler

Fixes #11
Fixes #13
Fixes #14
Fixes #15
Missing docs and more arguments.

Coverage decreased (-3.7%) to 38.292% when pulling bb6916cfd6ecfdbb6eb8ca7051fb9572f73f19c7 on feat/command-handler into 7688a0ae5422bdd5f32e9b068ad7636ab443adee on main.


Deleted outdated tutorial link in comment

Solves https://github.com/foxssake/netfox/issues/186

Merged, thanks!


SELECT * does not work correctly with GROUP BY

Search before asking

[X] I have searched the EvaDB issues and found no similar bug report.

Bug
SELECT * with GROUP BY does not output grouped columns.

Environment
No response
Are you willing to submit a PR?

[ ] Yes I'd like to help by submitting a PR!

Taking this as there might be a possibility that could be a common issue with https://github.com/georgia-tech-db/evadb/issues/1170 . If not, I'll update it


Fixed calling .wire with document

document has a special nodeType (9), and is always a parent node, so we need to call _wireElementsFrom when using it.

:ship:


CLI Migrations that make commits should do so as the CLI

🚀 Feature request
Command (mark with an x)

- [x] update

Description
Similar to ng-new, the CLI should commit with itself as the author instead of the user, making it easier to differentiate user commits from automatic commits.
CC @vikerman

@StephenFluin, the CLI will use which every git config you have.  If you have the a global git config with your use, When using ng new it will create the initial commit with your username.
I think the feature that you are talking about was in Angular CLI 1.6, where the author of the initial commit was Angular CLI.
I am not sure of all the reasons why this was changed.
//cc @clydin & @filipesilva maybe they remember.

Can't say I remember either.

It was user requested.  I believe the argument was that the CLI user didn't actually exist so it could cause validation failures and also that the commit ultimately was created by the developer as the developer executed the ng command.

now that the commits are authored only if the the user opts-in via --createCommits flag, I think it's not that surprising that the commits are authored by the current user.
in the past we used to author the initial git commit using a custom CLI author, and we rolled that back in CLI version ~1.4 because it was causing problems for teams that required all commits to be created by existing/real users.
@StephenFluin do you agree that the rationale for not using the current user to create the commits is no longer valid?

SGTM


USB HID is undocumented

A super minimal implementation exists. It only needs to be documented.

It was - just the link was missing. Resolved here: 57a5299e779f0f121eac4da66cfd4c465f582cc2


Refactored DropwizardAppRule support running different EnvironmentCommands

I'm working on writing a gRPC EnvironmentCommand for running gRPC in Dropwizard and needed the ability to be able to use a DropwizardAppRule for testing.
This pull request adds constructor parameters to DropwizardAppRule and DropwizardTestSupport so you can specify a Function that will instantiate the EnvironmentCommand that's run from DropwizardTestSupport. ServerCommand was used everywhere as a default to maintain backwards compatibility.

Coverage decreased (-0.004%) to 82.355% when pulling d2adea3294d87084bc44f64778ea8eb09a6219d7 on tburch:AlertnateCommandsInDropwizardAppRule into e3d6c3743b02397ea62752984cf2e8c6fc751f88 on dropwizard:master.

Coverage decreased (-0.004%) to 82.355% when pulling d2adea3294d87084bc44f64778ea8eb09a6219d7 on tburch:AlertnateCommandsInDropwizardAppRule into e3d6c3743b02397ea62752984cf2e8c6fc751f88 on dropwizard:master.

Coverage decreased (-0.004%) to 82.355% when pulling d2adea3294d87084bc44f64778ea8eb09a6219d7 on tburch:AlertnateCommandsInDropwizardAppRule into e3d6c3743b02397ea62752984cf2e8c6fc751f88 on dropwizard:master.

LGTM
Does your application override Application#addDefaultCommands(Bootstrap) to swap out the default ServerCommand? This brings up an interesting question: what is the recommended path for running a non-Jetty/HTTP Dropwizard server?
Another option would be augment ServerCommand to support server types beyond Jetty. This will be relevant to a future Dropwizard release which includes HTTP/2 support.

Coverage decreased (-0.004%) to 82.355% when pulling b804e41d9dbcd0cce0245f87611546b041368629 on tburch:AlertnateCommandsInDropwizardAppRule into e3d6c3743b02397ea62752984cf2e8c6fc751f88 on dropwizard:master.

Coverage decreased (-0.004%) to 82.355% when pulling b804e41d9dbcd0cce0245f87611546b041368629 on tburch:AlertnateCommandsInDropwizardAppRule into e3d6c3743b02397ea62752984cf2e8c6fc751f88 on dropwizard:master.

@evnm it does modify Application#addDefaultCommands. I first went down the path of refactoring ServerCommand, but then decided that what I wanted to do was different enough to warrant another command. I also noticed that dropwizard-http2 took this approach, so decided it was ok. I'll opensource it at some point (or I would happy to add it to Dropwizard)...
It looks like DropwizardTestSupport is pretty tightly coupled to Jetty, so it might make sense to abstract all of this a little (I saw #846 that looked like what may be needed).

Coverage decreased (-0.004%) to 82.355% when pulling 77e5d83261abec1c2324434335106ab8cb056094 on tburch:AlertnateCommandsInDropwizardAppRule into e3d6c3743b02397ea62752984cf2e8c6fc751f88 on dropwizard:master.

Gotcha, thanks for clarifying. I admittedly haven't looked into dropwizard-http2 yet.
gRPC support would be a great candidate for a 3rd-party module. To date, Dropwizard has been staunchly focused on HTTP services, so maintaining official support for other protocols would warrant a wider discussion.

I need something similar for testing a separate command for an application, so this comes in handy. Now, could this also use ConfiguredCommand instead of EnvironmentCommand?

@Ferada I don't see why not. I've updated the pull request.

Why not go all the way and have the function produce Commands?

Coverage decreased (-0.004%) to 82.357% when pulling 0725d42a8f99c76c6ca014a420c9e9fe599ca5bb on tburch:AlertnateCommandsInDropwizardAppRule into de8c7e3b9e1b98dfd1a13c9ce562209cb4e6f5f1 on dropwizard:master.

Coverage decreased (-0.004%) to 82.357% when pulling 0725d42a8f99c76c6ca014a420c9e9fe599ca5bb on tburch:AlertnateCommandsInDropwizardAppRule into de8c7e3b9e1b98dfd1a13c9ce562209cb4e6f5f1 on dropwizard:master.

Coverage decreased (-0.004%) to 82.357% when pulling 696c8125e63d5ff3d7fa7bf29246b82c1bd61dd9 on tburch:AlertnateCommandsInDropwizardAppRule into de8c7e3b9e1b98dfd1a13c9ce562209cb4e6f5f1 on dropwizard:master.

@jplock what are the chances of this being in a point release instead of 1.1.0? It's the same functionality as previous versions with an additional constructor to support additional functionality.

If we're going to be strict, given that the new constructor exposes additional functionality, this change would warrant a minor version bump, according to the precepts of semantic versioning.
I think that the best we can offer right now is to clarify a roadmap for the next few releases. From this, we can all rally around a specific task list for changes that are to be included in the next release.

Anything new regarding Dropwizard + GRPC integration? Is there, at least, a way to swap the Jetty server to another one?

@pparth checkout https://github.com/tburch/dropwizard-grpc

@tburch Looks good! Nice! thanks!


Independent menu scrolling

Fixes #405

Hey @begriffs I presume this PR is still WIP?

Ha, very much so. 😄

@sumedhpathak it's tricky to get the sidebar menu to stay in position without that persistent gap appearing above it on scroll. That's because it's position: fixed and starts out offset to make room for our top banner. I experimented with position: sticky which is a newer feature of CSS that is supposed to do what we want. Having trouble there because it causes the content next to it to "clear" below it.
One observation is that part of our difficulty is our custom theme where we have the Citus logo and horizontal menu above everything. The sidebar scrolls independently on read the docs sites that use the default sphinx theme. We might consider going back to the default theme and just putting the citus logo on our index page. In fact the horizontal menu style in the docs no longer matches our web site ever since the site redesign.

OK, check out the preview. I simplified the theme to fix the menu scrolling. Also removed the redundant TOC from the index page and put four sections as an idea of how we might guide people. If you like the content there I can turn it into a nice looking grid.
One thing I can also look into is showing a scrollbar on the menu maybe. See if you feel it provides enough visual cue that the menu can scroll.

Since there were issues with making the scrolling fix work with the header as it is, here is two quick variations that we could do, which look nicer (in my opinion) and we can make the scrolling work there:

@lfittl Do we have these deployed somewhere as a preview? We could share with Claire/Craig to get their feedback from a branding perspective as well.

They're not deployed -- we adjusted the properties live in the DOM in Chrome and took screenshots of the results. No actual code was changed or saved.

Yeah, this was just a quick hack - no easy way to persist that. I would recommend sharing the screenshots as discussion starters.


Use elemental's build-iso command instead of luet-makeiso

Signed-off-by: David Cassany<EMAIL_ADDRESS>

@bk201 this PR is adding in elemental binary a build-iso command which pretends to be luet-makeiso substitution, little by little we are moving most of the build and runtime operations over the elemental client, so we provide a more unified and consistent user experience. This PR is changing the manifest.yaml (which was used by luet-makeisp) and the iso related Makefile targets. Are you using any of these two in Harvester? Note that luet-makeiso is not dropped it is simply unused as in this PR Makefile targets are changed use elemental build-iso and the manifest.yaml adapted to the new syntax.
Iso syntax for elemental build-iso is something like:
iso:
  rootfs:
    - system/cos
  uefi:
    - live/grub2-efi-image
  isoimage:
    - live/grub2
    - live/grub2-efi-image
  label: "COS_LIVE"

name: "cOS-0"
date: true

@davidcassany Thanks for the pinging. We use luet-makeiso with a YAML file in our repo:
luet-makeiso iso.yaml --image "${HARVESTER_OS_IMAGE}" --output ${ARTIFACTS_DIR}/${ISO_PREFIX}

The YAML file: iso.yaml
If luet-makeiso still works as usual then it should work for us. Thanks again. cc @johnliu55tw

Looking great! nice!

If luet-makeiso still works as usual then it should work for us. Thanks again. cc @johnliu55tw

Indeed this what I expected, you use your own yaml file for the ISO and directly call luet-makeiso, so the change should simply not affect you in any way. Once merged we can iterate on how to adapt to the new builder. No big changes, just the command call is slightly different and the manifest is simplified, but the essence is the same.

Just rebased from master and updated elemental to include latest version with proper repository configuration.


[Suggestion]Translate tutorials-video_similarity_search.md into CN

Note: This repository is ONLY used to solve issues related to DOCS.
For other issues, please move to other repositories.

Is there anything that's missing or inappropriate in the docs? Please describe.
video_similarity_search.md
Describe your suggestion
CN translation needed.
Additional reference
Machine translation post editing (MTPE) is okay, but pure machine translation will not be accepted.
You should have a certain level of proficiency in both Chinese and English in order to take this issue.

I would like to work on this @YiyunNi

/assign


⚠ Bofrost.fr has degraded performance

In c48c6d6, Bofrost.fr (https://www.bofrost.fr) experienced degraded performance:

HTTP code: 200
Response time: 1644 ms

Resolved: Bofrost.fr performance has improved in 0a3eb94 after 6 minutes.


Android: make lld the default linker, since the Android NDK just removed all other linkers

This is just bringing over the one C++ Driver change from a larger compiler pull, so I don't have to change the linker manually all the time.

@swift-ci please test

@swift-ci please test


zero copy cast dict array to view type arrays

Which issue does this PR close?

Part of  #5861 .
Rationale for this change
Casting from DictionaryArray to String/BinaryView was previously handled by unpack_dictionary: https://github.com/apache/arrow-rs/blob/master/arrow-cast/src/cast/dictionary.rs#L93-L105
which incurs unnecessary copy to the value buffer. This pr handles Utf8View and BinaryView so that it will reuse the value buffer instead of creating a new one.

What changes are included in this PR?

Are there any user-facing changes?
No

BTW, with this pull request, casting a dictionary array to string view with 10_000 items improved performance by 25%
dict to view            time:   [37.979 µs 37.990 µs 38.001 µs]
                        change: [-25.271% -25.253% -25.237%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
  2 (2.00%) low mild
  5 (5.00%) high mild
  4 (4.00%) high severe

My plan is to merge this branch up to main, run the benchmarks added in https://github.com/apache/arrow-rs/pull/5874 on it and then assuming they look good merge it in

The bechmark I ran suggests this kernel is almost 2x as fast as the existing approach (which makes sense as it avoids a copy).
Nice work @XiangpengHao
++ critcmp master view-to-dict
group                       master                                 view-to-dict
-----                       ------                                 ------------
cast dict to string view    1.79    123.2±1.68µs        ? ?/sec    1.00     68.8±2.76µs        ? ?/sec
cast string view to dict    1.00    249.9±0.68µs        ? ?/sec    1.07    268.3±0.53µs        ? ?/sec


Provide means to manually activate a FingersCrossedHandler

This is useful if you have a FingersCrossedHandler set up and some where in your application are interested in all output regardless of whether the threshold is actually reached. For example to temporarily debug some problem. One can replace the handler in that spot, but this is signifcantly easier to use.

You still need to push out some log though after calling this, so I wonder if it's really much better than to just send a dummy error to force it to write out? Considering the latter doesn't add edge-casey code that is.

@Seldaek well I have alerting on my logs, and these definitely are not errors, so I'd rather not get paged over it. I just want to force the output, so I can track down background information in logs.

I'd be fine with automatically writing through on activate() and stopping buffering depending on the internal settings though.

Yeah I think it'd be better to have activate() basically act more like the BufferHandler flush(), which I guess is what you described. It'd remove the hackish flag, and make it also not need a new log record to trigger. Win/win.

@Seldaek fixed the two things you mentioned

Yay


Change test domain name to one that has an MX record

Note that this is very very brittle and really should be reworked.
Fixes #534

PHP 5.5 and 5.6 looks good:
https://travis-ci.org/zendframework/zf1/jobs/56080032
https://travis-ci.org/zendframework/zf1/jobs/56080033

All pass: https://travis-ci.org/zendframework/zf1


ToTimeUnit() in Elasticsearch.Net converts to years

Should ToTimeUnit() still convert TotalMilliseconds to years?
It currently doesn't convert to months and It seems we should also not convert to years, converting to weeks as the largest time unit.

:+1: It should match the logic in [Time.Reduce()[(https://github.com/elastic/elasticsearch-net/blob/master/src/Nest/CommonOptions/TimeUnit/Time.cs#L155).


Headers and footers are not appearing

There's an issue with headers and footers not appearing. It might be related to camel casing. Ferrum does some strange processing of the snake case params to convert them automatically to the camel cased parameters that chrome is expecting. But it's unclear if that is the issue.
Further, in Grover based experiments, i also found that chrome itself is pretty picky on the styles required on the html snippet.
There's a test script (manual) under test/headers_and_footers.rb.

@koenhandekyn This works:
Palapala::PDF.new("
Hello world</>").save("test.pdf", displayHeaderFooter: true, footerTemplate: '
Generated with Palapala PDF
')

pass the options to save, and in the footerTemplate you HAVE to specify the font-size. Otherwise is set to 0, because... reasons.

yeah thanks. if you passed the params on the save it worked as these params where passed as is through to the library. there was a typo in the params from the constructor.
there's a new version that is another order faster, it basically does the web socket communication directly. feel free to test it on e.g. claims. the API stayed the same and the initializer bug is fixed.


FP16 model recognized as FP8 by your GGUF converter

Hi,
I used your GGUF converter in tools to get this finetuned checkpoint to Q8_0.
This model has two files:

"FP16" 11.08GB
"BF16" 20.34GB

Afaik, FP16 and BF16 should have the same filesize, but it's not the case there, so I guess the uploader made a mistake, and the 11GB file is FP8 and the other is FP16/BF16.
But the strange thing is both files are getting converted from torch.float8_e4m3fn by your tool !
Is this an error on the model author ?
Maybe he finetuned a FP8 version of Flux.1 Dev but saved it as FP16/BF16, so that explains the missing precision ? Even then some precision should be gained from the finetune itself, so I don't think it's that
Or is this an error on your converter ?
Maybe letting us specify the input checkpoint floating point precision when using converter.py would help in this strange case ?
I'm not smart enough to analyze this model in details
Thanks for your help

Just looking at the files size and knowing the size of the official Flux.1 dev FP16 checkpoint, I can assume his 11GB file is definitely FP8.
But the second checkpoint, 20GB, is near the size of the FP16 original Flux.1 dev file, so it's really weird it's getting picked up as float8_e4m3fn and not FP16/BF16.
Maybe we have to manually check if the 20GB file last mantisses are just zeroed out, that would prove it's an issue on the model author side, saving useless data without added precision

That's actually an fp8 model with an fp16 text encoder and a bf16 vae all mixed together. If you check it manually you'll see that the actual model part is fp8.
import torch
from safetensors.torch import load_file
sd = load_file("STOIQONewrealityFLUXSD_F1DPreAlpha.safetensors")
dt = [v.dtype for k,v in sd.items()]
ct = {x:dt.count(x) for x in set(dt)}
print(ct)

{torch.float16: 416, torch.float8_e4m3fn: 780, torch.bfloat16: 244, torch.float32: 2}
for k,v in sd.items():
    print(v.dtype, k)

Oh I see, using your tool I only saw fp8 and nothing else iirc
Strange its still 20gb and the model part is only fp8
I'll ask the author if they can release a full fp16/bf16 version I guess
Thanks
Le mar. 24 sept. 2024 à 00:43, City @.***> a écrit :

That's actually an fp8 model with an fp16 text encoder and a bf16 vae all
mixed together. If you check it manually you'll see that the actual model
part is fp8.
import torch
from safetensors.torch import load_file
sd = load_file("STOIQONewrealityFLUXSD_F1DPreAlpha.safetensors")
dt = [v.dtype for k,v in sd.items()]
ct = {x:dt.count(x) for x in set(dt)}
print(ct)
{torch.float16: 416, torch.float8_e4m3fn: 780, torch.bfloat16: 244,
torch.float32: 2}
for k,v in sd.items():
print(v.dtype, k)
image.png (view on web)
https://github.com/user-attachments/assets/27c88a22-38b5-4e16-90e7-1af276a485ee
—
Reply to this email directly, view it on GitHub
https://github.com/city96/ComfyUI-GGUF/issues/112#issuecomment-2369673184,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AZVYVBQT6F4LHYCK56BVVHLZYCKR3AVCNFSM6AAAAABOVD2XYOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRZGY3TGMJYGQ
.
You are receiving this because you authored the thread.Message ID:
@.***>


Question: Publication Unblock

Can you please confirm that the following is indeed an issue?
In IpcPublication::checkForBlockedPublisher,
because initially we set
timeOfLastConsumerPositionChange = 0;
and
consumerPosition = producerPosition();
lastConsumerPosition = consumerPosition;
When we transition from having no subcription to having one, it triggers a call to  LogBufferUnblocker.unblock even if the publication is not block.
In my use case, the first claimed message is forcefully published by unblock when in fact it should be done via commit.
Maybe setting the timeOfLastConsumerPositionChange=currenttime when a new subscription is added would prevent this.
I can also increase the unblock timeout. However it would be a a very high value given that the difference between current time and timeOfLastConsumerPositionChange = 0 could be very high.

Following is a scenario leading to this situation:

Create a new publication (consumerPosition=0, lastConsumerPosition=0, producerPosition=0)
Create a new subscription (joinPosition=0)
Publication::tryClaim(X Bytes) producerPosition=X

We now enter the following function with:
consumerPosition=lastConsumerPosition=0
and
producerPosition() > consumerPosition
and
timeNs > (timeOfLastConsumerPositionChange + unblockTimeoutNs)
with timeOfLastConsumerPositionChange =0
So as soon as timeNs > unblockTimeoutNs, we call LogBufferUnblocker.unblock.
private void checkForBlockedPublisher(final long timeNs)
{
if (consumerPosition == lastConsumerPosition)
{
if (producerPosition() > consumerPosition &&
timeNs > (timeOfLastConsumerPositionChange + unblockTimeoutNs))
{
if (LogBufferUnblocker.unblock(termBuffers, metaDataBuffer, consumerPosition))
{
unblockedPublications.orderedIncrement();
}
}
}
else
{
timeOfLastConsumerPositionChange = timeNs;
lastConsumerPosition = consumerPosition;
}
}

I changed the following code and I no longer have the issue. However, I don't know if there's a better fix to that.
public void onTimeEvent(final long timeNs, final long timeMs, final DriverConductor conductor)
{
if (subscriberPositions.length == 0)
{
timeOfLastConsumerPositionChange = timeNs;
}
checkForBlockedPublisher(timeNs);

I noticed this possibility as well while adding unblock logic to the C media driver. But haven't had time to test it out.
Same possibility for network publications might also exist.
Also, replay publications are being worked on for replay. I didn't want to touch this until @mjpt777 and @nitsanw are happy with the changes that were involved with that.
The fix you mentioned is not quite correct, I don't think. It won't work for network publications is one reason. Another is that it is only the initial check call that has the issue as it has not had a chance to block for the timeout period.

My understanding is that this problem is still present for the first message published after a transition between having no subscribers and having at least one subscriber.
Ex:

A publication had subscriber but now have none.
a possible case is this: consumerPosition = lastConsumerPosition = producerPosition
timeOfLastConsumerPositionChange = T0

Wait unblockTimeoutNs

Add a new subscription

Claim a message but don't commit

At this point, we have

consumerPosition = lastConsumerPosition < producerPosition
timeOfLastConsumerPositionChange = T0
and timeNs = T0+unblockTimeoutNs
So we call LogBufferUnblocker.unblock and the publication claimed message is forcefully commited by the media driver.

Interesting issue this. It is different for IPC and network publications. The network publication is easily fixed using the timestamp on construction. For IPC it is more complicated as the consumers can come and go.
The timestamp can be reset when transitioning from 0 to 1 IPC subscribers. This will however reset the clock on a timeout if a publication had really become blocked before the subscriber got added.

Upon reflection (perhaps without enough coffee...) I'm not entirely sure that this is limited to a subscriber change on IPC.
timeOfLastConsumerPositionChange is only updated upon last consumer position and current position being different (or an added subscriber now).
So, it would seem to be the case that this sequence could occur.

consumerPosition = lastConsumerPosition = producerPosition (idle publication)
timeOfLastConsumerPositionChange = T0

Wait for instance 2* unblockTimeoutNs

Claim a message but don't commit for a brief (< unblockTimeoutNs) period

checkForBlockedPublisher occurs before commit of message

At this point, consumerPosition = lastConsumerPosition < producerPosition
timeOfLastConsumerPositionChange = T0 and timeNs >= T0+ 2*unblockTimeoutNs
This looks more like a race of the checkForBlockedPublisher and the commit on an idle IPC publication.
Am I missing something or just not enough time for coffee to fully kick in?
I have a big sense of Deja Vu with this.... I remember it when I put in the original unblock check. But can't see how it gets avoided now.

I concur this will solve this issue.
If I take into account the amount of money I paid to get Aeron in the first place (0$ ;p) I shouldn't complain but yet I will respectfully.
From what I understand, if an IPC subscriber stops consuming messages for more than the unblock period then the MediaDriver will start to unblock the IPC Publisher.
The same thing happens if a publisher takes for than the unblock period between claim and commit.
Also for the Subscriber's Image, if a Subscriber takes more than the lingering period to read messages then it will crash because the Image and Log Buffers it reads are deleted.
This is the type of things that should be documented (if possible avoided) because it is like juggling with razor blades as it makes the system going into an undefined behavior.
It would be better if we would only free resources or unblock messages only if we know it is safe to do so.

If we reset the timeout on each check if the consumer position has not changed but the producer has not advanced then it seems like we would be OK. Does this now work? Also means we don't need to care about number of subscribers.

@goglusid The Subscriber will not crash in the case you mentioned because it still has its own independent mapping that cannot be removed.

@mjpt777 I will test/review this later today or tomorrow.

@mjpt777 From what I understand the image mapping is removed after the lingering period. @tmontgomery ?

On the client, the Images are reclaimed after being lingered, yes.

@tmontgomery  The following line in bold destroys the mapping for a given image after the lingering timeout. So if during this period, we have an IPC subsription scanning the log buffers then it will crash. That's what happens when I debug for instance.
void ClientConductor::onCheckManagedResources(long long now)
{
std::lock_guardstd::recursive_mutex lock(m_adminLock);
// erase-remove idiom

// check LogBuffers
auto logIt = std::remove_if(m_lingeringLogBuffers.begin(), m_lingeringLogBuffers.end(),
  [now, this](const LogBuffersLingerDefn& entry)
{
  return (now > (entry.m_timeOfLastStatusChange + m_resourceLingerTimeoutMs));
});

**m_lingeringLogBuffers.erase(logIt, m_lingeringLogBuffers.end());**

Yes. It is marked closed and removed from the subscription so further usage is safe while in linger. Even after that the object when polled, if referenced directly, will return 0.

Yes, as I said, after the linger. The assumption is that the app will not block for an inordinate amount of time (i.e. longer than the linger timeout) in its poll.

@mjpt777  Indeed if you call poll after the close/removeImage  you are safe. However if you  call poll prior to these calls and don't return from poll during the lingering period then it will crash.
Obviously, you can put a huge timeout to cover most case but still relying on timeouts for this is risky.

@goglusid agreed. It is less than ideal. If you have a solution that does not rely on an atomic increment (and subsequent optimizations being avoided) on each subscription poll, I am very interested.

@tmontgomery  I know, ideally we need to figure out an approach that doesn't need to use a shared_ptr increment/decrement of atomic for each poll. I'm thinking of it.

Our best solution currently is the linger one. Sadly. It's a tradeoff of performance for more app responsibility (not blocking for linger timeout on the poll).
Another tradeoff is to single thread the client like is done in the Java version with the AgentInvoker. I do plan to port something close to that into the C++ API, though. That avoids the issue, but also means the app now takes on more responsibility (to call the client conductor duty cycle timely enough).

@tmontgomery Ok what about this. Given a subscription is only accessed by one thread, we could call periodically a fct ex Subcription::safePoint to notify the ClientConductor that we aren't in the poll function anymore. Safepoint could increment an atomic...not frequently though. When removeImage is called we would take a copy of this safePoint counter and later the ClientConductor would compare if the current safePoint is > than its copy, if so then it is safe to delete managed resources.
Basically, the responsability of knowning the safePoint is on the Subsriber side. If you accept to use more memory then you can call safePoint every hour.

@goglusid have to try it. I tried early on an RCU technique that was similar. Anything that stops migration within the Image loop cancels a host of optimization opportunities. Once it is outside that loop (like the loads for the array and the number of images), it is much better. However, that's the tricky bit. Needs to be within the loop to be useful.

@mjpt777 Right now I can't see why your latest changes wouldn't work. (double negation) ;p That being said lock free programming leads to an increase of the possible states your program can reach so who knows... In my test, it only unblocks if the timeout is reached. Thank you very much

@tmontgomery Using a C++ AgentInvoker for the ClientConductor would be very useful to make the lingering of resources safe.
I'll think about a way to notify the MediaDriver when it is safe to delete lingering resources. Stay tuned!

@goglusid FYI. See 33c1033f6 for AgentInvoker support in C++.

@tmontgomery That was fast Thanks

@tmontgomery In the mean time I implemented the following to make sure deleting resources is safe. Note that it only works for single threaded application. I added a produderId and consummerId both atomic values. When the media driver add a new resource to linger it increase the producer id, When the application calls a new fonction on the Aeron instance, it sets the consummer id = producer id. The media driver monitors the consummer id. when it is for each resource waiting to be deleted. when consummerID >= Resource.copyOfProducerId then it is safe to delete that resource.

@tmontgomery BTW using AgentInvoker as a way to make sure deleting resource is safe also implies having all the subscribers run a single thread.

@goglusid correct.

@mjpt777
Could you please confirm that the following use case isn't covered:
If a process calling a publication::offer crashes just before setting the new term's index then the media driver doesn't unblock this publication.
private long newPosition(final int index, final int currentTail, final long position, final long result)
{
....
else if (termOffset == TermAppender.TRIPPED)
{
final int nextIndex = nextPartitionIndex(index);
initialiseTailWithTermId(logMetaDataBuffer, nextIndex, termId(result) + 1);
        **>>>>> Publication's Process Crashes Here <<<<<<<**

        activePartitionIndexOrdered(logMetaDataBuffer, nextIndex);
    }

    return newPosition;
}

private void checkForBlockedPublisher(final long timeNs)
{
final long consumerPosition = this.consumerPosition;
     **>>>>> producerPosition() == consumerPosition so we don't call unblock <<<<<**

    if (consumerPosition == lastConsumerPosition && producerPosition() > consumerPosition)
    {
        if ((status == Status.INACTIVE) || (timeNs > (timeOfLastConsumerPositionUpdateNs + unblockTimeoutNs)))
        {
            if (LogBufferUnblocker.unblock(termBuffers, metaDataBuffer, consumerPosition))
            {
                unblockedPublications.orderedIncrement();
            }
        }
    }
    else
    {
        timeOfLastConsumerPositionUpdateNs = timeNs;
        lastConsumerPosition = consumerPosition;
    }
}

@goglusid I think you are correct. I'll create a test case and follow this up.

In order to test that I simply commented the line
activePartitionIndexOrdered(logMetaDataBuffer, nextIndex);
That way it emulates a crash and the following calls to offer all fails given that the term rotation is lost.

I've taken a different approach and allowed any publishing thread to perform the rotation. This will also be better if the thread doing the rotation gets interrupted never mind dying. I've found one other issue which I'll address separately.

I am curious to know more about this other issue.

Imagine the case of a large number of publishers all failing to complete the rotation. The driver needs to detect this and then unblock after the flow control window is exhausted. Quite a complicated one but ultimately possible.

@mjpt777
There's another problem:
Let's assume a lot of threads simultaneously calling Pulication::offer checking and passing the publication limit condition and all entering the TermAppender::appendUnfragmentedMessage. Given that each thread will increment the rawTail and the fact that the rawTail only has 32 bits for the offset, it is possible that the offset grow larger than 2^32 and start to mess with the upper bound of the rawTail (the termId). Do you agree this is an issue?
Another issue of using only 32 bits for the TermId is that you are limited to termLength*2^32 Bytes/Publication. This also mean that you now need to deal with a new Maxposition error  in the client.
In order to deal with the previous issues, I changed the way to manage the rawTail by bitshifting the termId according to the number of bits needed for the offset based on the term length. This allow 2^63 Bytes/Publication.  Also because the rawTail can no longer keep the termId I added another 64 bits to store the beginPosition of a term (TermId << bishift). I would be willing to Skype you to discuss in more details.

@goglusid Yes these are issues we are aware off and practically they are not an issue. On overflowing the first 32 bits we would need a number of threads all to be offering the maximum message length all to have passed the publication limit at the same time and be able to wrap. Have you done the calculation on how many threads it is for the different term lengths and message lengths?
The reaching max position is similar in that it is a LOT of data with anything but the smallest term lengths. Then it is a close the stream and open a new session to continue. We could look at an automated close in this case. However I'd be very interested in discussing alternative solutions to this.

@mjpt777
For a termlength of 1GB that is only 4 threads offering 1GB message. It is practically possible. No?

No :-) For the network publication max message length is 1/8 of term length. So that is 4 * 8 = 36.

I meant IPC Publication. Still 36 threads isn't that much.

It is the same for IPC.

I also think 1/8 GB for a single message is a bad design. Does not stream well. We should pragmatically put a cap on that. @tmontgomery What do you think of a 16MB message cap?

So if a publication is 32 bytes from term tripping and 36 threads tries to offer the max msg at the same time then we end up in a wrong state? You don't think this is practically an issue?

I think 16MB is more than generous.

At a 16MB cap it is very pragmatic.

Everything in computing has practical limits. Is 36 threads reasonable? Maybe. But is sending 128MB messages reasonable.... no. It's not. If you NEED 128MB messages, you are making the wrong tradeoffs.

At some point, any messaging system has to push back the responsibility of chunking to the application. 16MB is a practical limit that most applications will fit well within. And besides, once messages become too big, the application MUST assess the tradeoffs and decide for itself.

Any message that does not fit in less than 1/2 cache size is a questionable design. Bigger than that should be in chunks and be streamed.

< 16MB fits in with the wait-free tradeoff. You want wait-free, you use smaller messages. You want convenience of obscenely large messages, then I think giving up wait-free and doing chunking at the application level is a good tradeoff.

@tmontgomery
:) I like your "obscenely large messages" lol
With a max of 16 MB messages then it is more practically impossible to encounter the issue that I saw. Maybe it could be documented so if someone change this limit it may cause overflow in the rawTail.

For the C++ side, we can definitely do a compile time check for the value and break the build if the limit is changed.

We could have the driver enforce a limit on the number of addPublication to a given logbuffer as well.

Note that if we were to store offset as a multiple of 32 Bytes instead of #Bytes then we would avoid the overflow even more...even with bigger messages ;p Indeed the offset is a multiple of 32 bytes so there are 5 bits available.

@goglusid That is a nice observation on storing the multiple of 32. Maybe we should combine that with the message limit which would encourage better design anyway.

Also note that if you are obsene enough to send huge messages then you can always use an ExclusivePublication. That is still without having issue.


Updates to libensemble-rsmpi script

Fixes for #90 and #100

I'm going to go ahead and merge this. I've been using this for a few weeks now and it seems to be working well, and it is highly beneficial for monitoring jobs. There is no sign of deadlocks, but it is something keep in mind should issues arise with rsmpi execution.


Custom build without Template7 or Dom7

I appreciate that it is possible to do a custom build of Framework7 with gulp custom -accordion,modals,tabs but is it possible for Framework7 to work without Template7 or Dom7 ? If yes, how ? I don't have experience with gulp so I don't know which file should be modified.
Thank you in advance for your time and help.

No, sorry, it is not possible

Okey, I guess they are used internally. What if I want to use vuejs (or Riot.js/KnockoutJS/etc) instead of Template7 ? They both use mustache binding, is there a risk of conflict ?

If you don't use Template7 and don't enable it in your app, you won't get any conflicts

Just to be sure :
var myApp = new Framework7({
    template7Pages: false
});

And I'm good to go ?

Yes, it is disabled by default :)

Thank you very much, back to coding !


SAK-46737 Lessons Tool doesn't recognize Gradebook Classic

I'm not opposed to this, but why are we supporting this ancient gb tool?

We're still using it, but I wouldn't know how many other schools still do.  It's still available in the sakaicontrib github.  But if it's agreed that this PR shouldn't be merged, that's OK too.  (We'll just apply the fix locally)


nixpkgs manual: python section: comments on missing tkinter section

https://github.com/NixOS/nixpkgs/blob/d6d7259c5c982d6dec2a1a14a2c47cbe7dfa6d98/doc/languages-frameworks/python.section.md?plain=1#L25-L27

Make this stand out more with an note admonition.
The section could be reframed as a python.pkgs reference section, where the separation of tkinter inside it is raised as a note admonition.

Would it be possible to wrap the relevant commands so print Nix-specific instructions to stderr when running Python commands which require tkinter? That way users won't have to know about this limitation or this section of the documentation in advance.


Added reset option for perdown

#33
Usage in HTML:

Or in JS:
$("#timer").percircle({
    perdown: true,
    reset: true
});

Demo:

I also moved some of the timer code to separate functions so they become reusable.  Name-wise, hopefully they are intuitive enough.

Nice one @Sylphony , thank you!!!

No problem, thanks for the merge.  Keep up the awesome work!


Fixes 10e18 bug

Transforms incoming and outgoing 10e18 values.

Responded to comments and added guards to only allow digits and whole numbers in the amount input.


Include badges in project and pyansys-advanced

Resolves #54 by including the following badges:

Codecov Report

Merging #69 (9bffa87) into main (981c263) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #69   +/-   ##
=======================================
  Coverage   85.43%   85.43%
=======================================
  Files           6        6
  Lines         103      103
=======================================
  Hits           88       88
  Misses         15       15

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 981c263...9bffa87. Read the comment docs.

Requesting @akaszynski review to have feedback on the PyAnsys shield.


shell_plus notebook settings Import error in subdirectory

I have an issue where when I run python manage.py shell_plus --notebook I get an error where it can't find my Django settings when I navigate and create a notebook in a subdirectory of the project:
WARNING: Attempting to work in a virtualenv. If you encounter problems, please install IPython inside the virtualenv.
[IPKernelApp] WARNING | Error in loading extension: django_extensions.management.notebook_extension
Check your config files in /home/eric/.ipython/profile_default
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/IPython/core/shellapp.py", line 269, in init_extensions
    self.shell.extension_manager.load_extension(ext)
  File "/usr/local/lib/python2.7/dist-packages/IPython/core/extensions.py", line 86, in load_extension
    if self._call_load_ipython_extension(mod):
  File "/usr/local/lib/python2.7/dist-packages/IPython/core/extensions.py", line 133, in _call_load_ipython_extension
    mod.load_ipython_extension(self.shell)
  File "/usr/local/lib/python2.7/dist-packages/django_extensions/management/notebook_extension.py", line 5, in load_ipython_extension
    style=no_style())
  File "/usr/local/lib/python2.7/dist-packages/django_extensions/management/shells.py", line 102, in import_objects
    setup()
  File "/usr/local/lib/python2.7/dist-packages/django/__init__.py", line 17, in setup
    configure_logging(settings.LOGGING_CONFIG, settings.LOGGING)
  File "/usr/local/lib/python2.7/dist-packages/django/conf/__init__.py", line 48, in __getattr__
    self._setup(name)
  File "/usr/local/lib/python2.7/dist-packages/django/conf/__init__.py", line 44, in _setup
    self._wrapped = Settings(settings_module)
  File "/usr/local/lib/python2.7/dist-packages/django/conf/__init__.py", line 92, in __init__
    mod = importlib.import_module(self.SETTINGS_MODULE)
  File "/usr/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
ImportError: No module named MY_DJANGO_APP.settings.common

It works when I create a python notebook from the root directory of my project, but not from any subdirectory.
Any suggestions?

If you haven't resolved the issue you can start the notebook shell from the subdirectory where your notebooks are.

This issue is still unresolved, yes. But thanks for the tip in the interim.

@crearc Did you really manage to open/edit notebooks while using Django Shell Plus kernel? And Jupyter sees Django project and all the components while working in a subfolder? How can I do that? I asked here but I was not able to resolve it: http://stackoverflow.com/questions/34929528/django-and-ipython-subdirectories

Yes I was able to get them open/editable, but as I said the pynbs only worked from the root directory, in other directories the kernel did not work. But yeah I was able to import all my libraries just fine.
I did add some config stuff, not sure if it was necessary though:
settings.py
IPYTHON_ARGUMENTS = [
    '--ext', 'django_extensions.management.notebook_extension',
]
NOTEBOOK_ARGUMENTS = [
    '--config=jupyter_notebook_config.py',
]

jupyter_notebook_config.py
c.NotebookApp.ip = '*'
c.NotebookApp.open_browser = False # This was so it didn't try to open a browser in production
c.NotebookApp.port = 8888

Other than that there wasn't much else that I added.

Hold on. :o) Let me ask you clearly: are you having the same issue like me? Or can you import models etc from Django when working in subdirectory? As I explained, I tried to copy all notebooks to subdirectory called Calculations, all notebooks contain imports from Django models etc., but this does not work (cannot import Django stuff). So for now, I have to keep them in Django root directory... so many files, it gets pretty messy there.

Ahh yeah, I thought you couldn't get notebooks working at all. Yes I am
having the exact same issue and have not resolved it.
On Thu, Jul 7, 2016, 1:53 PM klimanek<EMAIL_ADDRESS>wrote:

Hold on. :o) Let me ask you clearly: are you having the same issue like
me? Or can you import models etc from Django when working in subdirectory?
As I explained, I tried to copy all notebooks to subdirectory called
Calculations, all notebooks contain imports from Django models etc., but
this does not work (cannot import Django stuff). So for now, I have to keep
them in Django root directory... so many files, it gets pretty messy there.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/django-extensions/django-extensions/issues/865#issuecomment-231173125,
or mute the thread
https://github.com/notifications/unsubscribe/AAYHiBlLdwl4GS5q8cUDsmy879UEoQQxks5qTUtGgaJpZM4H_j_a
.

Same problem here.

+1 would love a solution to this problem - i have like 150 notebooks in the root dir of my project, it's getting out of hand

I'm also having this problem. 😦

I managed to work around this problem, in case it's useful for anyone else. Basically, I added an ipython_config.py file to my notebook sub-directory, then manually inserted my base project directory into sys.path.
Project layout:
/project
    /someapp
    /notebooks
        ipython_config.py
    settings.py

settings.py
NOTEBOOK_ARGUMENTS = [
    '--notebook-dir', 'notebooks',
]

IPYTHON_ARGUMENTS = [
    '--debug',
]

notebooks/ipython_config.py
import sys
import os

FILE_PATH = os.path.abspath(os.path.dirname(__file__))
PROJECT_BASE_PATH = os.path.abspath(os.path.join(FILE_PATH, '..'))

# Allows the kernel to "see" the project during initialization. This
# FILE_PATH corresponds to Jupyter's "notebook-dir", but we want notebooks to
# behave as though they resided in the base directory to allow for clean
# imports.
print("sys.path BEFORE = {}".format(sys.path))
sys.path.insert(1, PROJECT_BASE_PATH)
print("sys.path AFTER = {}".format(sys.path))

# Any additional initialization logic goes here

In the logs, you should observe IPython locating that ipython_config.py file in your subdirectory. For example:
[D 13:28:12.612 NotebookApp] Starting kernel: [u'/usr/bin/python', u'-m', u'ipykernel', u'-f', u'MY_USER_HOME/.local/share/jupyter/runtime/kernel-....json', u'--debug', u'--ext', u'django_extensions.management.notebook_extension']
[D 13:28:12.616 NotebookApp] Connecting to: tcp://<IP_ADDRESS>:45969
[I 13:28:12.616 NotebookApp] Kernel restarted: f2c2cfa1-680e-4b17-bd95-03e1affb9b77
[D 13:28:12.617 NotebookApp] Connecting to: tcp://<IP_ADDRESS>:52312
[IPKernelApp] IPYTHONDIR set to: MY_USER_HOME/.ipython
[IPKernelApp] Using existing profile dir: u'MY_USER_HOME/.ipython/profile_default'
[IPKernelApp] Searching path [u'MY_PROJECT_BASE_PATH/notebooks', u'MY_USER_HOME/.ipython/profile_default', '/usr/local/etc/ipython', '/etc/ipython'] for config files
[IPKernelApp] Attempting to load config file: ipython_config.py
[IPKernelApp] Looking for ipython_config in /etc/ipython
[IPKernelApp] Looking for ipython_config in /usr/local/etc/ipython
[IPKernelApp] Looking for ipython_config in MY_USER_HOME/.ipython/profile_default
[IPKernelApp] Looking for ipython_config in MY_PROJECT_BASE_PATH/notebooks
sys.path BEFORE = ['', '/usr/lib/python2.7', '/usr/lib/python2.7/plat-x86_64-linux-gnu', '/usr/lib/python2.7/lib-tk', '/usr/lib/python2.7/lib-old', '/usr/lib/python2.7/lib-dynload', '/usr/local/lib/python2.7/dist-packages', '/usr/lib/python2.7/dist-packages', '/usr/local/lib/python2.7/dist-packages/IPython/extensions', 'MY_USER_HOME/.ipython']
sys.path AFTER = ['', MY_PROJECT_BASE_PATH, '/usr/lib/python2.7', '/usr/lib/python2.7/plat-x86_64-linux-gnu', '/usr/lib/python2.7/lib-tk', '/usr/lib/python2.7/lib-old', '/usr/lib/python2.7/lib-dynload', '/usr/local/lib/python2.7/dist-packages', '/usr/lib/python2.7/dist-packages', '/usr/local/lib/python2.7/dist-packages/IPython/extensions', 'MY_USER_HOME/.ipython']
[IPKernelApp] Loaded config file: MY_PROJECT_BASE_PATH/notebooks/ipython_config.py
[IPKernelApp] Attempting to load config file: ipython_kernel_config.py
[IPKernelApp] Looking for ipython_kernel_config in /etc/ipython
[IPKernelApp] Looking for ipython_kernel_config in /usr/local/etc/ipython
[IPKernelApp] Looking for ipython_kernel_config in MY_USER_HOME/.ipython/profile_default
[IPKernelApp] Looking for ipython_kernel_config in MY_PROJECT_BASE_PATH/notebooks
...
[IPKernelApp] Starting the kernel at pid: 209
...
[IPKernelApp] Loading IPython extensions...
[IPKernelApp] Loading IPython extension: storemagic
[IPKernelApp] Loading IPython extension: django_extensions.management.notebook_extension
[IPKernelApp]
*** MESSAGE TYPE:kernel_info_request***
[IPKernelApp]    Content: {}
   --->
...

If anybody wants to create a PR with documentation update, I'm happy to merge it.
Otherwise closing this for now :-)


Reduce unnecessary downloads and reinstalls

Hi,
First of all, thank you for the awesome plugin!
I am using the version 6.0.0 of the plugin with yarn 3.1.0. I noticed that some tasks run and download tools at every build, which make builds somewhat slow. I came up with some workarounds as shown below. I am unsure how useful they would generally be for the plugin, but if any of them are interesting, I am willing to try and create a PR with similar functionality (although I'm a beginner in gradle plugin writing, so that may take some time).
Nodejs is downloaded every time the classpath of the installNode task changes
I am using precompiled script plugins from the buildSrc directory to enforce some project-wide conventions. Every time I change a convention .gradle file, the buildSrc project is recompiled, which changes the classpath of the installNode task (since it comes from the implementation classpath of my buildSrc project, transitively). This results in node being redownloaded even if the desired node version wasn't changed.
I couldn't really come up with a clean workaround for this, short of storing the installed node version explicitly in a .properties file:
https://github.com/kris7t/refinery/blob/a530fe054944ca28bb49985c4334de20bca9c24c/buildSrc/src/main/groovy/refinery-frontend-worktree.gradle#L10-L62
Yarn 1 is downloaded at every build
Running the npm install -g yarn command at every build seems to trigger an installation even if yarn 1 was already installed. Moreover, the repeated installs mean that yarn 1 is updated to the latest version every time, which may hurt reproducibility.
As a workaround, we may pin a specific yarn 1 version and store it in a .properties file to see whether we need to reinstall:
https://github.com/kris7t/refinery/blob/a530fe054944ca28bb49985c4334de20bca9c24c/buildSrc/src/main/groovy/refinery-frontend-worktree.gradle#L6
https://github.com/kris7t/refinery/blob/a530fe054944ca28bb49985c4334de20bca9c24c/buildSrc/src/main/groovy/refinery-frontend-worktree.gradle#L64-L72
I guess both this and the previous workaround is generally useful unless the user is using a global node installation (when no node or yarn 1 should be downloaded automatically).
We download and switch to yarn berry at every build
As far as I could determine, switching to yarn berry is required before switching to a specific version if there is no .yarn/versions/yarn-{version}.cjs is the repository because yarn 1 will just refuse to switch to a specific version otherwise.
However, yarn 3 recommends storing the yarn-{version}.cjs in the repository anyways, so we can get away with disabling the enableYarnBerry task:
https://github.com/kris7t/refinery/blob/releng-docs/buildSrc/src/main/groovy/refinery-frontend-conventions.gradle#L12-L14
I am unsure how generally useful this workaround is. Maybe we should explicitly recommend storing yarn 3 in the repository (as upstream does), or at least provide a configuration option whether the user wants to re-create .yarn at every build (as opposed to upstream recommendation) or not?
We download and switch to the desired yarn version at every build
Since yarn 3 is usually stored in the repository, there is no need to re-download it at every build. The exception is when the desired yarn 3 version changes compared to what's in the repository.
Luckily, the yarn 3 version is in its filename, so there's no need to cache it in a .properties file. Instead, we can just mark it as an output of the installYarn task:
https://github.com/kris7t/refinery/blob/a530fe054944ca28bb49985c4334de20bca9c24c/buildSrc/src/main/groovy/refinery-frontend-worktree.gradle#L74-L76
We can further reduce re-downloads (for example, in installYarn is forced to re-run due to classpath changes) by using yarn set version --only-if-needed:
https://github.com/kris7t/refinery/blob/a530fe054944ca28bb49985c4334de20bca9c24c/buildSrc/src/main/groovy/refinery-frontend-worktree.gradle#L7
We reinstall npm packages at every build
This doesn't cause superfluous downloads, because yarn caches all packages, but installFrontend is still a bit slow when it re-calculates package resolutions even if no package.json or yarn.lock file changed.
Thanks to pnp resolution, the installed state of packages is fully described by the .pnp.cjs and .pnp.loader.mjs files. Compared to traversing the node_modules directory (which was the norm with npm and yarn 1), checking the dates on these files can be done quickly so we can mark them as inputs and outputs:
https://github.com/kris7t/refinery/blob/a530fe054944ca28bb49985c4334de20bca9c24c/buildSrc/src/main/groovy/refinery-frontend-worktree.gradle#L78-L81
This workaround is only useful if pnp resolution is used. Perhaps we could add a configuration option for that, too?
This introduced a lot of caching. I wanted to provide an easy escape hatch if something breaks (although hopefully nothing will), so I added a clobberFrontend task that nukes the node a .yarn directories:
https://github.com/kris7t/refinery/blob/a530fe054944ca28bb49985c4334de20bca9c24c/buildSrc/src/main/groovy/refinery-frontend-worktree.gradle#L83-L91
What to keep / not keep was based on the recommendations in the yarn documentation.

Hi,
Thank you for these interesting questions/suggestions. It is extremely difficult to deal efficiently with all these suggestions with a single issue. Please can you open a dedicated issue for each of the 5 topics you mentioned so as we can focus on each one independently?
Thanks in advance, and thank you for using the plugin 😊.
BR

Just for documentation, I added the following to my build.gradle.kts:
frontend {
    nodeVersion.set("14.17.5")
    yarnEnabled.set(true)
    yarnVersion.set("3.1.1")
    assembleScript.set("run codegen")
}

tasks.installYarnGlobally {
    outputs.dir("node/lib/node_modules/yarn")
}

tasks.enableYarnBerry {
    inputs.property("yarnVersion", frontend.yarnVersion)
    outputs.file(frontend.yarnVersion.map { ".yarn/releases/yarn-$it.cjs" })
}

tasks.installYarn {
    inputs.property("yarnVersion", frontend.yarnVersion)
    outputs.file(frontend.yarnVersion.map { ".yarn/releases/yarn-$it.cjs" })
}

tasks.installFrontend {
    inputs.file("package.json")
    inputs.file("yarn.lock")
    outputs.dir("node_modules")
}

tasks.assembleFrontend {
    inputs.file("codegen.yml") // project dependent
    inputs.file("package.json")
    inputs.file("yarn.lock")
    inputs.files(fileTree("src/main/resources/") { // project dependent
        include("**/*.graphqls")
    })
    outputs.dir("build/graphql-code-generator/kotlin") // project dependent, required
}

I use the plugin to run yarn run codegen, to run the graphql-code-generator in my Kotlin/Java project.

@tristanlins I'm not sure this could work with yarn.
As a reminder gradle can cache tasks only if inputs and outputs are properly documented, and there's no overlap between tasks.
A task is cacheable if it is annotated by @CacheableTask or via the API outputs.cacheIf { true }.
The task installYarnGlobally will run npm install -g yarn do not have a version, so one may end up with a newer version.
And depending on where the destination is set the task might not be cacheable
And when the enableYarnBerry is run then the global version lands in the files in the current dir
.yarn/releases/yarn-3.3.0.cjs
.yarnrc.yaml        (yarnPath: .yarn/releases/yarn-3.3.0.cjs)

But then the installYarn task runs it will use the version defined in the frontend section.
Which may not be the same as the version coming from the global install. E.g. this will create a file
.yarn/releases/yarn-3.1.0.cjs

But this will "polute" the folder and on the next run gradle will notice the output has changed (overlap), and this will make the task run again.

Hello @kris7t,
Hereafter you will find some news about your questions.

Nodejs is downloaded every time the classpath of the installNode task changes

The Node.js distribution is downloaded again because some content in the buildSrc directory is modified, whatever it is. As stated in Gradle docs: "A change in buildSrc causes the whole project to become out-of-date.". This is a behaviour by design and the plugin will not workaround this. I understand this may be frustrating on your side, but what about refactoring your build and move parts you are frequently updating outside the buildSrc directory?
Below you will find some articles I found, explaining why the buildSrc should be replaced by composite builds, for the exact same problem (build cache invalidation):

https://proandroiddev.com/stop-using-gradle-buildsrc-use-composite-builds-instead-3c38ac7a2ab3
https://medium.com/bumble-tech/how-to-use-composite-builds-as-a-replacement-of-buildsrc-in-gradle-64ff99344b58

Gradle docs here also mention an interesting improvement: "Making the implicit buildSrc project an included build.", which, as I understand it, could solve this problem in your case.

Yarn 1 is downloaded at every build
We download and switch to yarn berry at every build
We download and switch to the desired yarn version at every build

Starting from release 7+ and the use of Node.js corepack utility, the optimization is now implemented.

We reinstall npm packages at every build

This has been discussed in issues #180, #60, and in the official documentation of task installFrontend. The plugin abstracts installation and use of supported package managers. Moreover, depending on the package manager, inputs and outputs are not the same, and may vary depending on the project and the state of the file system. I understand this can be frustrating not to have a built-in optimization during install. With my actual knowledge, it appears to me too much complex to abstract this optimization, so as it works in all possible configurations.
However, since this point has been questioned many times:

The documentation of task installFrontend now explain what could be inputs and outputs for each package manager, to help configure optimization depending on the project and the build case. But this does not replace official documentation of each package manager.
Examples were refactored and introduce a configuration proposal of a basic and naive optimization for each package manager. These are probably not ideal solutions and I will be happy to read suggestions for all-purpose improvements.

Thanks to pnp resolution, the installed state of packages is fully described by the .pnp.cjs and .pnp.loader.mjs files

I didn't find some documentation about the .pnp.loader.mjs file. I'll be happy to learn more if you can share some official notes.

Given your questions and these clarifications, I close the issue. But discussion may continue if needed or if I'm mistaken. Thank you for your help!
BR


Changing BPM fucks up chart editor.

Describe your bug here.
When I attempted to change the BPM of my song, all sections afterwards either had no notes or their notes compressed above the top of the section, even though they play back perfectly fine in level. You can even still place more notes in those sections as normal, but no matter how many times I reloaded, the screwed notes wouldn't fix unless I undid the BPM change.
Pretty sure this is a bug, I have attached a video of the phenomenon below.
https://github.com/ShadowMario/FNF-PsychEngine/assets/157259571/001447b6-efdf-46db-b42d-5e04b23f7b82
Command Prompt/Terminal logs (if existing)
No response
Are you modding a build from source or with Lua?
Lua
What is your build target?
Windows
Did you edit anything in this build? If so, mention or summarize your changes.
I have changed nothing in this build.

when you change the bpm, it changes the amount of strums that can hold a note.
Lets say you have a bpm of 100 with notes at steps 20, 40, and 60. If you were to change the bpm to 120, there is suddenly a 20% increase in the beats per second. The notes are still at steps 20, 40, and 60, but if the section the notes were in happened to change (if step 20 was in section 2, then after the bpm change it might become section 1), then it results in what happened with you: being that the notes are only visible in the section they were originally placed in.
FIX: You need to change the bpm back, and find a way to note what step the notes are in. You basically have to clear the chart, change the bpm, then place the notes in the same step, in the correct section.

Damn, thanks dude, that's super annoying when you already have 100's of notes placed lmao. In that case Shadow should add a fix where you can reload the arrows to whatever step they are assigned to fix it. It's still kind of an issue, so I'll leave dis open for now.

when you change the bpm, it changes the amount of strums that can hold a note.
Lets say you have a bpm of 100 with notes at steps 20, 40, and 60. If you were to change the bpm to 120, there is suddenly a 20% increase in the beats per second. The notes are still at steps 20, 40, and 60, but if the section the notes were in happened to change (if step 20 was in section 2, then after the bpm change it might become section 1), then it results in what happened with you: being that the notes are only visible in the section they were originally placed in.
FIX: You need to change the bpm back, and find a way to note what step the notes are in. You basically have to clear the chart, change the bpm, then place the notes in the same step, in the correct section.

when you change the bpm, it changes the amount of strums that can hold a note.
Lets say you have a bpm of 100 with notes at steps 20, 40, and 60. If you were to change the bpm to 120, there is suddenly a 20% increase in the beats per second. The notes are still at steps 20, 40, and 60, but if the section the notes were in happened to change (if step 20 was in section 2, then after the bpm change it might become section 1), then it results in what happened with you: being that the notes are only visible in the section they were originally placed in.
FIX: You need to change the bpm back, and find a way to note what step the notes are in. You basically have to clear the chart, change the bpm, then place the notes in the same step, in the correct section.

Correction: If you change the BPM, the notes won't be at the same step. The notes are all measured by time, grouped by 4 beat sections. If a section has notes and those notes have times corresponding to another section, they will all chill at the top or at the bottom. There are ways to fix this but you have to get technical (aka, do some coding)

bruh i had this issue today it's so annoying. im going to have to either clear my chart or try and do it with my previous notes which is kinda bad

bruh i had this issue today it's so annoying. im going to have to either clear my chart or try and do it with my previous notes which is kinda bad
i'd also like to point out shadow mario saw this but didn't put a fix to this?

Because it's already implemented in the 1.0 chart editor rewrite.

BETTER FIX: open the json file in notepad, scroll down to the very bottom, and change the bpm, works just fine after that

BETTER FIX: open the json file in notepad, scroll down to the very bottom, and change the bpm, works just fine after that

There shouldn't be an option to change the bpm if it doesn't work.


Scrollbar plugin calculates wrong max height

I'm using the jQuery Scrollbar plugin in 3 parallel DIV columns. The height of the columns always adapt to the height of the browser window. For some reason, the scrollbar height in the 2nd column is not calculated correctly. After some days of debugging I found out that one child DIV inside the 2nd column seems to trigger this issue.
It's the "item_cnt" DIV. Whenever setting this DIV to display:none, everything works fine. Unfortunately I need this DIV and cannot find a workaround how to make the scrollbar run with it.
I've created a fiddle.

You are doing wrong several things:

Set height to inner DIV instead of scrollable one
Set height via JS - use CSS instead
Use IDs instead of CSS classes (it's just advice to use classes instead of IDs)
Reinitialize custom scrollbar each time window is resized (there is no need)

Think about different page structure - header/content/footer, where content occupies all available space, and all content children elements (with classes, not IDs!) are scrollable and use 100% height of parent container. All of this can be done using pure CSS without JS. And then apply custom scrollbar to your content children elements only once, no need to handle window resize.

Thanks for your answer! You are right, I'm not really proud of the current HTML page structure. That's why I also worked on a newer and better one (without JS usage). But here I ran into the problem that the jQuery scrollbar wasn't shown:
Fiddle

https://fiddle.jshell.net/temo6yd6/2/
add to CSS: .scrollbar-dynamic { height: 100%; overflow: auto; }

Awesome! Thanks, thats exactly what I tried to do!


Tooltips on the rings

When you hover over the rings, it would be helpful to have a tooltip with the name of the class. Though this should probably be a toggle-able feature.

Im okay with making this a permanent feature, seems small enough


add tests demonstrating problems with riscv

@pauldreik This PR is likely to break under big-endian systems. I will merge and fix up the endian-dependency.

@pauldreik Note that converting from char* to char32_t is endian dependent.


Wrong comment format

Of the three comment format described, the last one (// This is a single line comment) does not seem to be interpreted as a comment

Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

ID: 0566a65b-8e82-4de1-0568-a6f28ae33d03
Version Independent ID: eb158bc3-f017-32e3-18ca-a20efe306d86
Content: Expression builder in mapping data flow - Azure Data Factory
Content Source: articles/data-factory/concepts-data-flow-expression-builder.md
Service: data-factory
GitHub Login: @kromerm
Microsoft Alias: makromer

@jeppe742 Thank you for the feedback. At this time we are reviewing the feedback and will provide an update as appropriate.

Hi @jeppe742 Thanks for reporting this to us. I have verified with internal team and confirmed that only block comments (/* comment */) are supported.
The PR for the fix has been created and it will be merged soon. Thanks again for pointing this out, I am closing the issue based on this update, please let us know if there is something else that we can help you with.


Calendar dates have wrong weekday

Hi,
the calendar seems to be off by one day. The 01.01.2017 is a Sunday but in your calendar its a Monday. Today is Tuesday the 03.01. and in the demo it is shown as a Wednesday. See:

Thank you @ma-schroeder. I will give it a look.

@ma-schroeder
Ok, I have found the root of this evil behaviour ;-). I will add fix to planned release v1.1 (this week).
If you need to fix it ASAP you have to edit the plugin PL/SQL in Shared components > Plugins > Pretius APEX Date Range section Source. Replace line 111
  lang_app_days_of_week       varchar2(200)  := getTranslation('PRETIUS_DATERANGEPICKER_DAYS'         , 'Mo,Tu,We,Th,Fr,Sa,Su');
with
  lang_app_days_of_week       varchar2(200)  := getTranslation('PRETIUS_DATERANGEPICKER_DAYS'         , 'Su,Mo,Tu,We,Th,Fr,Sa');
After saving, the plugin will display days names in correct correlation with day number in month

Thanks, this is working. But i had to set "First Day" to 2 in Component Settings:

its default since 1.2.0


Add top scroll button

Is your feature request related to a problem? Please describe.
Top scroll button
Describe the solution you'd like
I want to add top scroll button on 'Scripts section'.
@1e9abhi1e10 assign this issue to me under GSSoC'23

Not Needed


Run SonarCloud only on pull requests to master

Run SonarCloud less often to reduce build times for normal CI builds.

 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.:x: Brucius sign nowYou have signed the CLA already but the status is still pending? Let us recheck it.


chore(misc): Remove bip39 and use mnemonic utility function

What this PR does 📖

Removes the bip39 crate and use the mnemonic utility function built into warp

Which issue(s) this PR fixes 🔨

N/A

Special notes for reviewers 🗒
Additional comments 🎤

@dariusc93 can we just merge this into the other open PR for account-recovery?


Different monsters should have different dialog faces

Depending on the monster who's talking, their dialog faces should change. Blue monsters should have a blue face, fanged monsters should have fanged faces, etc...
Draw expressions for all of the different monsters, and try to make sure they overlay OK. While in the long run this could lead to an O(n^2) mess of facial combinations, where this guy's angry face with this type of eyebrow results in a visual glitch, I'm planning to sharply limit the different combinations.

Make sure this works for all faces, and all colors. The emote colors should usually be the same, but sometimes they'll be different.


Inconsistent metrics result between eval and denovo mode, and possible reason

This is a follow-up of issue  #111, thanks to all the information provided, I am able to find a possible bug and its reason.
I did experiment with weights "pretrained_excl_yeast.ckpt", deepnovo original raw yeast dataset, and Casanovo version 3.1.0.
In eval mode (results displayed on console):
aa_precision: 0.7057459339992785
aa_recall: 0.7071716086435299
pep_recall: 0.5046985050978404
In denovo mode (results manually calculated from output mztab file):
aa_precision: 0.6827751198617065
aa_recall: 0.6843725335842246
pep_recall: 0.5046985051027288
There is difference between results from eval and denovo mode (aa_precision, aa_recall), so I studies model.py and its pytorch lightning implementation.
I first excluded the possibility that this may be caused by predictions without '$' or with multiple '$$' (it turns out there are no such predictions), because this line of code removes the first position of prediction without checking whether it is '$', I am referring to v3.2.0, the code in v3.1.0 is similar.
Then I checked validation part of the code, and find the inconsistence might be caused by the way pytorch lightning accumulates metrics from each step and calculates per-epoch metrics. So I manually accumulates the prediction data from each step and calculate metrics for one epoch, and find it is the same as the results from denovo mode, which verifies my guess. For manual calculation, I use methods from evaluate.py.
Manual calculation from validation_step() output:
aa_precision: 0.6827751198617065
aa_recall: 0.6843725335842246
pep_recall: 0.5046985051027288
In order to further verify, I tried with deepnovo raw ricebean dataset, and see the same pattern.
In eval mode (results displayed on console):
aa_precision:     0.5835929214378351
aa_recall:      0.583695772587527
pep_recall:      0.4223428193208169
In denovo mode (results manually calculated from output mztab file):
aa_precision:     0.576437557764845
aa_recall:      0.5758105380102586
pep_recall:      0.4223428193248386
Manual calculation from validation_step() output:
aa_precision:     0.576437557764845
aa_recall:      0.5758105380102586
pep_recall:      0.4223428193248386
So the conclusion is, pytorch lightning can accumulate pep_recall from each step and calculate it correctly for each epoch, but it may fail the same job for aa_precision and aa_recall.
Ps. I again would like to request numerical values of Casanovo's AA/peptide precision/recall for updated 9-species benchmark (Figure S5 from preprint). Thanks a lot.

Thank you very much for the values!
For preprint Figure S2 and S5 only:

I assume the results are from Casanovo v3.2.0 trained with MASSIVEKB dataset and tested on each of the revised 9 species, with beam=10 and precursor window size=30ppm as mentioned in the preprint?

For Casanovo bm (Casanovo v2.1.1) in Figure S2 and S5, is it also trained with MASSIVEKB dataset, or it is trained with the other 8 species in the revised benchmark?

MASSIVEKB dataset has all 7 modifications, while current release of revised 9 species benchmark doesn't contain the 4 N-terminal modifications yet (#125). For these benchmark results, do you use exactly the same data as in this link without N-terminal modifications?

Thanks!

Yes
Casanovo_bm was trained on the older version of the benchmark, i.e. trained on 8 species from the older benchmark and tested on the remaining 9th species on the original, similar to DeepNovo.
The benchmark dataset these results were obtained from includes N-terminal modifications (see Figure S4 for results when a subset of PTMs that DeepNovo and Novor couldn't predict were excluded from the benchmark). See the update on #125 and check out the regenerated benchmark once it's available.

A similar situation has been discover in train mode, the "AA precision   AA recall   Peptide recall" of validation dataset displayed on console is also much higher than the actual accuracy, which might also be caused by pytorch-lightning codes in model.py. I would also suggest not use these values, I have compare them with manually calculated AA precision and peptide recall values from output mztab files, these displayed values are consistently higher.  They can only be used as an indicator of when to stop training but not accurate performance of benchmark dataset.

Thanks for pointing this out - we're also relying on manual calculation of precision values when reporting results

Notes from our discussion just now:
One hypothesis (from Wout) is that this may be due to a mismatch in the batch size, and it throws the calculation off.
Another is that it has to do with how Pytorch Lightning aggregates results at the level of epoch. But if that's the case, then others would have run into this as well.  Googling did not turn anything up.

Thanks for letting me know!

I am reopening this because we are still hoping to track down this discrepancy. It might not be immediate, though.

Although the values seem different, this should also have been impacted by #252. We should check again after merging #253.

I wrote a script to calculate the AA and Peptide precision of the Casanovo PSMs, and I am seeing a minor discrepancy between the output of my script and Casanovo's evaluate mode. I ran the script and evaluate mode on the sample data, which has no skipped spectra during sequencing, so the error is unlikely originating from that. From what I can tell the discrepancy is either from an error in my script or we're still seeing batch effects in eval mode. Here's the output from my script and eval mode:
(casanovo_env) C:\Users\gavin\OneDrive\Documents\casanovo>casanovo evaluate sample_data\sample_preprocessed_spectra.mgf -c data/casanovo.yaml
WARNING: Dataloader multiprocessing is currently not supported on Windows or MacOS; using only a single thread.
Seed set to 454
INFO: Model weights file C:\Users\gavin\AppData\Local\casanovo\casanovo_v420_v4_2_0.ckpt retrieved from local cache
INFO: Casanovo version 4.1.1.dev15+gcedfaa7.d20240625
INFO: Sequencing and evaluating peptides from:
INFO:   sample_data\sample_preprocessed_spectra.mgf
GPU available: True (cuda), used: True
TPU available: False, using: 0 TPU cores
HPU available: False, using: 0 HPUs
INFO: Reading 1 files...
sample_data\sample_preprocessed_spectra.mgf: 128spectra [00:00, 3208.59spectra/s]
LOCAL_RANK: 0 - CUDA_VISIBLE_DEVICES: [0]
Validation DataLoader 0:   0%|                                                                                | 0/1 [00:00<?, ?it/s]WARNING: UserWarning: 1Torch was not compiled with flash attention. (Triggered internally at ..\aten\src\ATen\native\transformers\cuda\sdp_utils.cpp:455.)
WARNING: UserWarning: 1Torch was not compiled with flash attention. (Triggered internally at ..\aten\src\ATen\native\transformers\cuda\sdp_utils.cpp:455.)
WARNING: D:\anaconda3\envs\casanovo_env\lib\site-packages\lightning\pytorch\utilities\data.py:78: Trying to infer the `batch_size` from an ambiguous collection. The batch size we found is 128. To avoid any miscalculations, use `self.log(..., batch_size=batch_size)`.

WARNING: D:\anaconda3\envs\casanovo_env\lib\site-packages\lightning\pytorch\utilities\data.py:78: Trying to infer the `batch_size` from an ambiguous collection. The batch size we found is 128. To avoid any miscalculations, use `self.log(..., batch_size=batch_size)`.

Validation DataLoader 0: 100%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 1/1 [00:03<00:00,  0.27it/s]INFO: Step       Train loss      Valid loss      Peptide precision       AA precision
INFO: 0 nan     0.376451        0.492188        0.745707
Validation DataLoader 0: 100%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 1/1 [00:03<00:00,  0.26it/s]
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃         Validate metric         ┃          DataLoader 0           ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩
│   AA precision at coverage=1    │       0.7457072734832764        │
│ Peptide precision at coverage=1 │            0.4921875            │
│          valid_CELoss           │        0.376451313495636        │
└─────────────────────────────────┴─────────────────────────────────┘
INFO: ======= End of Run Report =======
INFO: Run Start Time: 24/07/11 10:45:17
INFO: Run End Time: 24/07/11 10:45:37
INFO: Time Elapsed: 0:00:19.993431
INFO: Executed Command: D:\anaconda3\envs\casanovo_env\Scripts\casanovo evaluate sample_data\sample_preprocessed_spectra.mgf -c data/casanovo.yaml
INFO: Executed on Host Machine: DESKTOP-P03U1SR
INFO: Max GPU Memory Utilization: 381 MiB

(casanovo_env) C:\Users\gavin\OneDrive\Documents\casanovo>python val_bug_test.py
AA Precision: 0.7432542927167355
Pep Precision: 0.48437499996215816

Also, here is the test script:
import depthcharge.masses
import casanovo
import depthcharge
import pyteomics.mztab

import casanovo.denovo
import casanovo.denovo.evaluate

def process_mgf_file(mgf_path):
    with open(mgf_path) as f:
        return [
            line[4:].strip() for line in f if line.startswith("SEQ=")
        ]

def main():
    mz_tab = pyteomics.mztab.MzTab("foo.mzTab")
    output = mz_tab.spectrum_match_table["sequence"].tolist()
    ground_truth = process_mgf_file("sample_data\sample_preprocessed_spectra.mgf")
    peptide_masses = depthcharge.masses.PeptideMass()

    aa_precision, _, pep_precision = casanovo.denovo.evaluate.aa_match_metrics(
        *casanovo.denovo.evaluate.aa_match_batch(
            ground_truth,
            output,
            peptide_masses.masses
        )
    )

    print(f"AA Precision: {aa_precision}")
    print(f"Pep Precision: {pep_precision}")

if __name__ == "__main__":
    main()

Your script is correct, so I think it's still an issue with aggregating the results across multiple batches.
Maybe this can be relatively easily checked by running evaluation with a single batch only (by expanding the batch size and/or reducing the number of spectra). Although, come to think of it, the batch size for inference should standard be 1024, so maybe that's already the case. Could you double-check this?

I haven't looked at the code, but doesn't it seem a bit crazy to try to compute these metrics in a batchwise fashion?  Why not just aggregate a score vector and a label vector and then do a single call to the metric calculation at the end?

Now it's calculated at the end of each validation batch, where the loss is also calculated. Otherwise you have to keep all of the predictions and true labels in memory across the whole run, which can easily be on the order of millions. So in terms of code it's not really crazy but actually a natural way to do it. But maybe leading to slightly incorrect results then.

I checked the default config and bath size for inference does indeed default to 1024. As for the batch effects, I may be misunderstanding the amino acid and peptide precision scores but wouldn't be possible to keep a count of the number of correct amino acids and peptides, as well as the total number of amino acids and peptides, and use this values to do the precision scores calculation at the end of an evaluation run?

We should first verify what the actual problem is. This batched data aggregation was only my hypothesis, we haven't isolated the issue yet. You could debug the code I referred to a bit more to see what the values there are and try to find the point where they deviate.

The tentative implementation of #355 calculates the evaluation metrics in a way that is roughly equivalent to the test script. I'll reexamine this once #355 has been merged in.